From 785b48bca0c3450b4b20a54fd9bf7b4e56223b86 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 18:30:44 -0700 Subject: [PATCH 1/8] fix: Evaluate step-level let bindings in template scope A step-level EXPR `let` binding was evaluated with the host's path format. openjd-rs evaluates template-scope expressions with `PathFormat::Posix` (create_job/instantiate.rs, create_job/mod.rs) so that a create-time result cannot depend on the host that created the job. This implementation used the engine default, which is the host's format, so on Windows `startswith(path("/foo/bar"), "/foo")` evaluated to false where the Rust implementation gives true, and `string(path("/mnt/out"))` rendered `\mnt\out` rather than `/mnt/out`. `evaluate_let_bindings` now takes a `path_format`, and `StepTemplate._extend_step_symtab` passes `PathFormat.POSIX`. The default stays `None` (the engine default) so every session-scope caller is unchanged. The instantiated `StepScript` also records `_template_scope_let_count`, the number of leading entries in its merged `let` list that came from the step. The merge itself is unchanged, because consumers that do not pass a resolved symbol table to `Session.run_task` rely on it. openjd-sessions reads the count to re-evaluate that prefix in template scope rather than in the host's, and reads it through `getattr` with a default of 0, so an older openjd-model degrades to the previous behaviour. It is a `PrivateAttr`, so the serialized form of the model does not change. This is the openjd-model half of a two-repo fix. Without the openjd-sessions half, a step-level binding is still re-evaluated in host scope at session time, so the 11 Python-on-Windows conformance failures this addresses need both halves. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/_let_bindings.py | 13 +- src/openjd/model/v2023_09/_model.py | 54 ++++++- test/openjd/model_v0/test_let_bindings.py | 59 ++++++++ .../model_v0/v2023_09/test_let_bindings.py | 139 +++++++++++++++++- 4 files changed, 259 insertions(+), 6 deletions(-) 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..a6a4ad29 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -16,6 +16,7 @@ model_validator, ConfigDict, Discriminator, + PrivateAttr, StringConstraints, Field, PositiveInt, @@ -1094,6 +1095,18 @@ class StepScript(OpenJDModel_v2023_09): embeddedFiles: Optional[EmbeddedFiles] = None # noqa: N815 let: Optional[list[str]] = None + # RFC 0007 (EXPR): on an instantiated Step's script, how many leading `let` + # entries came from the step-level bindings that StepTemplate.resolve_syntax_sugar + # merged in ahead of the script's own. Those were already evaluated in + # template scope at job creation, so a session given the step's resolved + # symbol table must skip them rather than re-evaluate them in host scope — + # re-evaluating re-renders paths in the host's format, which changes results + # (RFC 0007 §3.6; openjd-rs evaluates template scope with PathFormat::POSIX). + # Private, so the model's serialized form does not change. Consumers read it + # with a getattr(script, "_template_scope_let_count", 0) guard, so an older + # model degrades to re-evaluating everything. + _template_scope_let_count: int = PrivateAttr(default=0) + _template_variable_scope = ResolutionScope.TASK _template_variable_definitions = DefinesTemplateVariables( symbol_prefix="|Task.", @@ -3392,6 +3405,21 @@ class Step(OpenJDModel_v2023_09): # for the task-run path. let: Optional[list[str]] = None + @model_validator(mode="after") + def _record_template_scope_let_count(self) -> Self: + """Record on the script how many of its leading `let` entries are the + step-level ones (RFC 0007). + + ``StepTemplate.resolve_syntax_sugar`` builds the script's `let` as + ``step-level bindings + the script's own``, in that order, and the + step-level ones are preserved verbatim here as ``self.let`` — so their + count is the length of the step-level list. A session that was handed + the step's resolved symbol table skips that many entries instead of + re-evaluating create-time results in host scope. + """ + self.script._template_scope_let_count = len(self.let) if self.let else 0 + return self + class StepTemplate(OpenJDModel_v2023_09): """Definition of a single Step within a Job Template. @@ -3447,6 +3475,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 +3491,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 = { @@ -3580,7 +3624,9 @@ def resolve_syntax_sugar(self) -> "StepTemplate": # 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. + # runtime seeds it when entering the step's environments, and + # Step._record_template_scope_let_count uses its length to mark + # where the template-scope prefix of the merged list ends. 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}) @@ -3622,7 +3668,9 @@ def resolve_syntax_sugar(self) -> "StepTemplate": ), # 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. + # are preserved into the Job and resolved at runtime. The + # step-level prefix length is recorded by + # Step._record_template_scope_let_count. let=([*(self.let or []), *(simple_action.let or [])] or None), embeddedFiles=[ EmbeddedFileText.model_construct( 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..74c00ae4 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -4,9 +4,17 @@ environments, host-context function scoping, and type-aware expression validation.""" +import json + import pytest -from openjd.model import DecodeValidationError, decode_job_template +from openjd.model import ( + DecodeValidationError, + SymbolTable, + create_job, + decode_job_template, + model_to_object, +) _EXTS = ["EXPR", "FEATURE_BUNDLE_1"] @@ -177,3 +185,132 @@ 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 TestTemplateScopeLetCount: + """The instantiated Step's script carries a merged `let` — the step-level + bindings first, then the script's own. `_template_scope_let_count` records + how many leading entries are the step-level ones, so a session handed the + step's resolved symbol table can skip re-evaluating them in host scope.""" + + @staticmethod + def _script(step, *, extensions=("EXPR",)): + template = _decode(_job([step], extensions=extensions)) + return create_job(job_template=template, job_parameter_values={}).steps[0].script + + def test_no_step_lets_is_zero(self): + script = self._script({"name": "S", "script": _onrun("hi")}) + + assert script._template_scope_let_count == 0 + assert script.let is None + + def test_script_lets_only_is_zero(self): + script = self._script({"name": "S", "script": {"let": ["a = 1"], **_onrun("{{a}}")}}) + + assert script._template_scope_let_count == 0 + assert script.let == ["a = 1"] + + def test_step_lets_only(self): + script = self._script( + {"name": "S", "let": ["a = 1", "b = 2"], "script": _onrun("{{a}}{{b}}")} + ) + + assert script._template_scope_let_count == 2 + assert script.let == ["a = 1", "b = 2"] + + def test_both_scopes_counts_only_the_step_level_ones(self): + script = self._script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + ) + + # THEN: the count is the step-level count, and the merged order puts the + # step-level bindings first — the two together are what makes a prefix + # skip correct. + assert script._template_scope_let_count == 2 + assert script.let == ["a = 1", "b = 2", "c = 3"] + + def test_syntax_sugar_script_records_the_count(self): + # The de-sugaring path builds its own merged `let`, so it needs the same + # boundary recorded. + script = self._script( + { + "name": "S", + "let": ["a = 1"], + "python": {"script": "print(1)", "let": ["b = 2"]}, + }, + extensions=("EXPR", "FEATURE_BUNDLE_1"), + ) + + assert script._template_scope_let_count == 1 + assert script.let == ["a = 1", "b = 2"] + + def test_count_is_private_and_not_serialized(self): + # Hard requirement: recording the boundary must not change the model's + # serialized shape. + step = { + "name": "S", + "let": ["a = 1"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + job = create_job(job_template=_decode(_job([step])), job_parameter_values={}) + + # WHEN + obj = model_to_object(model=job) + + # THEN + assert job.steps[0].script._template_scope_let_count == 1 + assert "_template_scope_let_count" not in json.dumps(obj) + assert obj["steps"][0]["script"]["let"] == ["a = 1", "c = 3"] From 48e91a3755e6ff23a7d0a9c4142df449619f99be Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:55:41 -0700 Subject: [PATCH 2/8] fix: Record the template-scope let boundary at the merge, and verify it Two defects in the previous commit, both found by local review. The boundary was recorded only by `Step._record_template_scope_let_count`, an after-validator on `Step`. A consumer that parses a `StepTemplate` and runs `resolve_syntax_sugar()` on it never constructs a `Step`, so the validator never ran and the boundary was lost. The Deadline worker agent does exactly that, via BatchGetJobEntity, so the fix reached `openjd-cli` and not the worker. Measured before: `create_job` gave a count of 2 and the resolve_syntax_sugar path gave nothing. The count is now set at the merge site as well, and both paths give 2. The validator also set the count to `len(self.let)` without checking that `script.let` actually starts with `self.let`. It runs for a `Step` built directly, where nothing guarantees that. A mismatch recorded a count that would make a session evaluate a genuinely session-scope binding in template scope. It now verifies the prefix and records 0 when it does not match, which is the previous behaviour. The attribute's comment claimed a session "must skip" the prefix. It reproduces it in template scope and re-tags the result to host format instead, so the comment is corrected to say that. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index a6a4ad29..b05b20e5 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -1098,9 +1098,9 @@ class StepScript(OpenJDModel_v2023_09): # RFC 0007 (EXPR): on an instantiated Step's script, how many leading `let` # entries came from the step-level bindings that StepTemplate.resolve_syntax_sugar # merged in ahead of the script's own. Those were already evaluated in - # template scope at job creation, so a session given the step's resolved - # symbol table must skip them rather than re-evaluate them in host scope — - # re-evaluating re-renders paths in the host's format, which changes results + # template scope at job creation, so a session must reproduce them in that + # scope rather than re-evaluate them in the host's — re-evaluating in host + # format re-renders paths, which changes results # (RFC 0007 §3.6; openjd-rs evaluates template scope with PathFormat::POSIX). # Private, so the model's serialized form does not change. Consumers read it # with a getattr(script, "_template_scope_let_count", 0) guard, so an older @@ -3413,11 +3413,18 @@ def _record_template_scope_let_count(self) -> Self: ``StepTemplate.resolve_syntax_sugar`` builds the script's `let` as ``step-level bindings + the script's own``, in that order, and the step-level ones are preserved verbatim here as ``self.let`` — so their - count is the length of the step-level list. A session that was handed - the step's resolved symbol table skips that many entries instead of - re-evaluating create-time results in host scope. + count is the length of the step-level list. + + The prefix is *verified*, not assumed. This validator also runs for a + ``Step`` built directly, where nothing guarantees that ``script.let`` + starts with ``self.let``; recording a count that does not match would + make a session evaluate a genuinely session-scope binding in template + scope. A mismatch records 0, which is the previous behaviour. """ - self.script._template_scope_let_count = len(self.let) if self.let else 0 + step_let = self.let or [] + script_let = self.script.let or [] + matches_prefix = bool(step_let) and script_let[: len(step_let)] == step_let + self.script._template_scope_let_count = len(step_let) if matches_prefix else 0 return self @@ -3629,6 +3636,13 @@ def resolve_syntax_sugar(self) -> "StepTemplate": # where the template-scope prefix of the merged list ends. merged_let = [*self.let, *(self.script.let or [])] new_script = self.script.model_copy(update={"let": merged_let}) + # Set here, at the merge, and not only on the instantiated Step. + # A consumer that resolves syntax sugar on a StepTemplate and + # runs the resulting script directly -- the worker agent does + # exactly this, via BatchGetJobEntity -- never constructs a + # Step, so Step._record_template_scope_let_count never runs for + # it and the boundary would be lost. + new_script._template_scope_let_count = len(self.let) return self.model_copy(update={"script": new_script}) return self From 30dd59d187174f044c1f22ba7fd1f76e9b7ff60d Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:22:19 -0700 Subject: [PATCH 3/8] chore: Refresh THIRD-PARTY-LICENSES for pydantic 2.13.5 The generator script fails on BSD sed, so the two version lines CI reported are bumped by hand: pydantic 2.13.4 to 2.13.5 and pydantic_core 2.46.4 to 2.46.5. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- THIRD-PARTY-LICENSES.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From 82af0642c0235de27e61bd71d80f2cec12642229 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 22:11:59 -0700 Subject: [PATCH 4/8] fix: Record the let boundary on the de-sugared script too StepTemplate.resolve_syntax_sugar builds the SimpleAction script with model_construct and never set _template_scope_let_count, while the script: branch does. For the same merged let list the sugar branch yielded 0 and the script: branch 1, so a consumer that parses a StepTemplate and calls resolve_syntax_sugar() directly -- the worker agent, via BatchGetJobEntity -- got no boundary and evaluated the step template's own let bindings in session scope instead of template scope. Set it at the merge, from the same source of truth (len(self.let)), by assignment, as the script: branch does, since model_construct bypasses validators. The existing coverage went through create_job, where Step's validator records the count regardless, so it did not reach this path. The new test resolves the sugar on the StepTemplate itself and compares against the script: branch, over all five interpreters. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 57 +++++++++++-------- .../model_v0/v2023_09/test_let_bindings.py | 35 ++++++++++++ 2 files changed, 67 insertions(+), 25 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index b05b20e5..81e776e2 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -3668,34 +3668,41 @@ 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, + ) + ), + # 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, + ) + ], + ) + if self.let: + # Same as the `script:` branch above: record the template-scope + # prefix length here, at the merge, so a consumer that resolves + # syntax sugar on a StepTemplate and runs the resulting script + # directly -- the worker agent does exactly this -- still gets the + # boundary. `model_construct` bypasses validators, so the private + # attribute is set by assignment, as it is there. + new_script._template_scope_let_count = len(self.let) 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. The - # step-level prefix length is recorded by - # Step._record_template_scope_let_count. - 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/v2023_09/test_let_bindings.py b/test/openjd/model_v0/v2023_09/test_let_bindings.py index 74c00ae4..75e1d3fc 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -247,6 +247,14 @@ def _script(step, *, extensions=("EXPR",)): template = _decode(_job([step], extensions=extensions)) return create_job(job_template=template, job_parameter_values={}).steps[0].script + @staticmethod + def _resolved_script(step): + """The script a consumer gets from ``resolve_syntax_sugar()`` alone — no + ``Step`` is constructed, so ``Step._record_template_scope_let_count`` + never runs.""" + template = _decode(_job([step], extensions=("EXPR", "FEATURE_BUNDLE_1"))) + return template.steps[0].resolve_syntax_sugar().script + def test_no_step_lets_is_zero(self): script = self._script({"name": "S", "script": _onrun("hi")}) @@ -314,3 +322,30 @@ def test_count_is_private_and_not_serialized(self): assert job.steps[0].script._template_scope_let_count == 1 assert "_template_scope_let_count" not in json.dumps(obj) assert obj["steps"][0]["script"]["let"] == ["a = 1", "c = 3"] + + @pytest.mark.parametrize("interpreter", ("bash", "python", "cmd", "powershell", "node")) + def test_syntax_sugar_records_the_count_without_a_step(self, interpreter): + # The worker agent parses a StepTemplate and calls resolve_syntax_sugar() + # directly, so no Step is built and the boundary has to be recorded at + # the de-sugaring merge — as the `script:` branch records it. + # GIVEN + sugar_script = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + interpreter: {"script": "print(1)", "let": ["c = 3"]}, + } + ) + script_branch = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + ) + + # THEN: both merge the same list, so both must record the same boundary. + assert script_branch.let == ["a = 1", "b = 2", "c = 3"] + assert script_branch._template_scope_let_count == 2 + assert sugar_script.let == ["a = 1", "b = 2", "c = 3"] + assert sugar_script._template_scope_let_count == script_branch._template_scope_let_count From 6a2381543967a46f61433bd925ab6b34c5eac20e Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:35:24 -0700 Subject: [PATCH 5/8] fix: Never lower the template-scope let marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Step._record_template_scope_let_count` wrote its computed count to `self.script._template_scope_let_count` unconditionally. The script is a shared object: one merged by `StepTemplate.resolve_syntax_sugar` can be reached by more than one `Step`, and the instance is kept rather than revalidated. A sibling `Step` whose own `let` is empty, or does not match the prefix, computed 0 and wrote it over a correct marker of 2 — silently reverting the owning step to re-evaluating template-scope bindings in host scope, the bug the marker exists to prevent. The write could only ever lower a correct value to a wrong one. Record the count only when it is non-zero. Two `Step`s share a script object only when they share its `let` list, so the marker the owning step computed describes that list correctly for both readers, whereas a 0 computed by a sibling describes only that sibling's own `let`. The existing `matches_prefix` verification still guards a genuinely mismatched prefix, so a non-zero count is only recorded after verification, and an unmarked script still reads 0 from the PrivateAttr default. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 21 ++++++- .../model_v0/v2023_09/test_let_bindings.py | 59 +++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 81e776e2..78f83d5e 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -3419,12 +3419,29 @@ def _record_template_scope_let_count(self) -> Self: ``Step`` built directly, where nothing guarantees that ``script.let`` starts with ``self.let``; recording a count that does not match would make a session evaluate a genuinely session-scope binding in template - scope. A mismatch records 0, which is the previous behaviour. + scope. A mismatch records nothing, leaving the script's existing marker + (0 unless some other step already recorded one -- see below). """ step_let = self.let or [] script_let = self.script.let or [] matches_prefix = bool(step_let) and script_let[: len(step_let)] == step_let - self.script._template_scope_let_count = len(step_let) if matches_prefix else 0 + count = len(step_let) if matches_prefix else 0 + # Only ever raise the marker, never lower it. A script merged by + # StepTemplate.resolve_syntax_sugar can be reached by more than one + # Step, and the same object is kept here rather than revalidated, so a + # sibling Step whose own `let` is empty or does not match the prefix + # computes 0 -- writing that over a correct marker would silently + # revert the owning step to the bug the marker exists to prevent. Two + # Steps share a script object only when they share its `let` list, so + # the marker the owning step computed describes that list correctly for + # both readers, whereas a 0 computed here describes only this Step's + # own `let`. `matches_prefix` above remains the guard against a + # genuinely mismatched prefix: a non-zero count is still only ever + # recorded after that verification. An unmarked script already reads 0 + # from the PrivateAttr default, so skipping the write leaves it at 0 + # rather than unset. + if count: + self.script._template_scope_let_count = count return self 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 75e1d3fc..ad540d9b 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -15,6 +15,7 @@ decode_job_template, model_to_object, ) +from openjd.model.v2023_09 import Step _EXTS = ["EXPR", "FEATURE_BUNDLE_1"] @@ -349,3 +350,61 @@ def test_syntax_sugar_records_the_count_without_a_step(self, interpreter): assert script_branch._template_scope_let_count == 2 assert sugar_script.let == ["a = 1", "b = 2", "c = 3"] assert sugar_script._template_scope_let_count == script_branch._template_scope_let_count + + @pytest.mark.parametrize( + "sibling_let", + (None, ["z = 9"]), + ids=("no-let", "non-matching-let"), + ) + def test_sibling_step_does_not_clear_a_shared_scripts_count(self, sibling_let): + # A merged script is a single object, and a Step keeps the instance it is + # given rather than revalidating it, so more than one Step can reach it. + # A sibling whose own `let` is empty or does not match the prefix + # computes 0 for itself; writing that over the marker the owning step + # recorded would revert the owner to re-evaluating template-scope + # bindings in host scope -- the bug the marker exists to prevent. + # GIVEN + script = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + ) + owner = Step(name="S", script=script, let=["a = 1", "b = 2"]) + assert owner.script is script + assert script._template_scope_let_count == 2 + + # WHEN + sibling = Step(name="T", script=script, let=sibling_let) + + # THEN + assert sibling.script is script + assert script._template_scope_let_count == 2 + + def test_step_records_its_own_count_and_an_unmarked_script_is_zero(self): + # Negative control for the "never lower the marker" rule: it must not + # stop a Step from recording the count its own `let` legitimately + # yields, and a script no Step has ever marked must read 0. + # GIVEN + marked = self._resolved_script( + { + "name": "S", + "let": ["a = 1", "b = 2"], + "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, + } + ) + unmarked = self._resolved_script({"name": "S", "script": _onrun("hi")}) + # The de-sugaring merge records on the merged script; strip it so this + # asserts the Step validator's own write, not the one already there. + fresh = marked.model_copy() + fresh._template_scope_let_count = 0 + + # WHEN + recording = Step(name="S", script=fresh, let=["a = 1", "b = 2"]) + never_marked = Step(name="U", script=unmarked) + + # THEN + assert recording.script._template_scope_let_count == 2 + assert unmarked.let is None + assert never_marked.script._template_scope_let_count == 0 From 260b147847256a7fad7254f98d3bbefb517716e7 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:01:44 -0700 Subject: [PATCH 6/8] fix: Stop merging step-level let into the script A step's template-scope `let` is already resolved at job creation and travels in the step symbol table returned by create_job_with_symbol_tables(...).step_symbol_tables[step_name]. For a step-level let of ["a = 1", "root = path('/foo/bar')", "txt = string(path('/mnt/out'))"] that table holds `a` int 1, `root` path '/foo/bar' stored POSIX, and `txt` string '/mnt/out', with the script-level let correctly absent. The worker agent forwards that table to openjd-sessions as of 08a5878b and deleted its extra_let_bindings channel outright. Merging the step-level let into script.let for the runtime to re-evaluate is therefore redundant, and harmful: when both run, the session-side re-evaluation writes last and clobbers the correctly formatted seeded value. Removed: - the merge in the `script:` branch of StepTemplate.resolve_syntax_sugar - the merge in the SimpleAction sugar branch - the Step after-validator _record_template_scope_let_count - the _template_scope_let_count PrivateAttr on StepScript The create-time step-scope evaluation that populates the step symbol table is unchanged, and Step.let remains a model field -- it just stops being merged into the script. Deleted TestTemplateScopeLetCount, which tested the removed machinery, and added tests pinning the new contract: resolve_syntax_sugar leaves script.let holding only the script's own bindings (both the `script:` branch and all five SimpleAction interpreters), the step-level bindings arrive in the step symbol table with a PATH value rendering /foo/bar under POSIX and \foo\bar under WINDOWS, and the script-level let is absent from that table. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/v2023_09/_model.py | 100 ++------- .../model_v0/v2023_09/test_let_bindings.py | 211 ++++++------------ 2 files changed, 79 insertions(+), 232 deletions(-) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 78f83d5e..229caef4 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -16,7 +16,6 @@ model_validator, ConfigDict, Discriminator, - PrivateAttr, StringConstraints, Field, PositiveInt, @@ -1095,18 +1094,6 @@ class StepScript(OpenJDModel_v2023_09): embeddedFiles: Optional[EmbeddedFiles] = None # noqa: N815 let: Optional[list[str]] = None - # RFC 0007 (EXPR): on an instantiated Step's script, how many leading `let` - # entries came from the step-level bindings that StepTemplate.resolve_syntax_sugar - # merged in ahead of the script's own. Those were already evaluated in - # template scope at job creation, so a session must reproduce them in that - # scope rather than re-evaluate them in the host's — re-evaluating in host - # format re-renders paths, which changes results - # (RFC 0007 §3.6; openjd-rs evaluates template scope with PathFormat::POSIX). - # Private, so the model's serialized form does not change. Consumers read it - # with a getattr(script, "_template_scope_let_count", 0) guard, so an older - # model degrades to re-evaluating everything. - _template_scope_let_count: int = PrivateAttr(default=0) - _template_variable_scope = ResolutionScope.TASK _template_variable_definitions = DefinesTemplateVariables( symbol_prefix="|Task.", @@ -3401,49 +3388,11 @@ 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 - @model_validator(mode="after") - def _record_template_scope_let_count(self) -> Self: - """Record on the script how many of its leading `let` entries are the - step-level ones (RFC 0007). - - ``StepTemplate.resolve_syntax_sugar`` builds the script's `let` as - ``step-level bindings + the script's own``, in that order, and the - step-level ones are preserved verbatim here as ``self.let`` — so their - count is the length of the step-level list. - - The prefix is *verified*, not assumed. This validator also runs for a - ``Step`` built directly, where nothing guarantees that ``script.let`` - starts with ``self.let``; recording a count that does not match would - make a session evaluate a genuinely session-scope binding in template - scope. A mismatch records nothing, leaving the script's existing marker - (0 unless some other step already recorded one -- see below). - """ - step_let = self.let or [] - script_let = self.script.let or [] - matches_prefix = bool(step_let) and script_let[: len(step_let)] == step_let - count = len(step_let) if matches_prefix else 0 - # Only ever raise the marker, never lower it. A script merged by - # StepTemplate.resolve_syntax_sugar can be reached by more than one - # Step, and the same object is kept here rather than revalidated, so a - # sibling Step whose own `let` is empty or does not match the prefix - # computes 0 -- writing that over a correct marker would silently - # revert the owning step to the bug the marker exists to prevent. Two - # Steps share a script object only when they share its `let` list, so - # the marker the owning step computed describes that list correctly for - # both readers, whereas a 0 computed here describes only this Step's - # own `let`. `matches_prefix` above remains the guard against a - # genuinely mismatched prefix: a non-zero count is still only ever - # recorded after that verification. An unmarked script already reads 0 - # from the PrivateAttr default, so skipping the write leaves it at 0 - # rather than unset. - if count: - self.script._template_scope_let_count = count - return self - class StepTemplate(OpenJDModel_v2023_09): """Definition of a single Step within a Job Template. @@ -3641,26 +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, and - # Step._record_template_scope_let_count uses its length to mark - # where the template-scope prefix of the merged list ends. - merged_let = [*self.let, *(self.script.let or [])] - new_script = self.script.model_copy(update={"let": merged_let}) - # Set here, at the merge, and not only on the instantiated Step. - # A consumer that resolves syntax sugar on a StepTemplate and - # runs the resulting script directly -- the worker agent does - # exactly this, via BatchGetJobEntity -- never constructs a - # Step, so Step._record_template_scope_let_count never runs for - # it and the boundary would be lost. - new_script._template_scope_let_count = len(self.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(): @@ -3694,10 +3630,10 @@ def resolve_syntax_sugar(self) -> "StepTemplate": 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), + # 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, @@ -3708,14 +3644,6 @@ def resolve_syntax_sugar(self) -> "StepTemplate": ) ], ) - if self.let: - # Same as the `script:` branch above: record the template-scope - # prefix length here, at the merge, so a consumer that resolves - # syntax sugar on a StepTemplate and runs the resulting script - # directly -- the worker agent does exactly this -- still gets the - # boundary. `model_construct` bypasses validators, so the private - # attribute is set by assignment, as it is there. - new_script._template_scope_let_count = len(self.let) return StepTemplate.model_construct( name=self.name, description=self.description, 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 ad540d9b..f407ede4 100644 --- a/test/openjd/model_v0/v2023_09/test_let_bindings.py +++ b/test/openjd/model_v0/v2023_09/test_let_bindings.py @@ -4,18 +4,15 @@ environments, host-context function scoping, and type-aware expression validation.""" -import json - import pytest +from openjd.expr import PathFormat from openjd.model import ( DecodeValidationError, SymbolTable, - create_job, + create_job_with_symbol_tables, decode_job_template, - model_to_object, ) -from openjd.model.v2023_09 import Step _EXTS = ["EXPR", "FEATURE_BUNDLE_1"] @@ -237,47 +234,21 @@ def test_step_symtab_path_predicate_is_host_independent(self): assert str(symtab["under"]) == "true" -class TestTemplateScopeLetCount: - """The instantiated Step's script carries a merged `let` — the step-level - bindings first, then the script's own. `_template_scope_let_count` records - how many leading entries are the step-level ones, so a session handed the - step's resolved symbol table can skip re-evaluating them in host scope.""" - - @staticmethod - def _script(step, *, extensions=("EXPR",)): - template = _decode(_job([step], extensions=extensions)) - return create_job(job_template=template, job_parameter_values={}).steps[0].script +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): - """The script a consumer gets from ``resolve_syntax_sugar()`` alone — no - ``Step`` is constructed, so ``Step._record_template_scope_let_count`` - never runs.""" template = _decode(_job([step], extensions=("EXPR", "FEATURE_BUNDLE_1"))) return template.steps[0].resolve_syntax_sugar().script - def test_no_step_lets_is_zero(self): - script = self._script({"name": "S", "script": _onrun("hi")}) - - assert script._template_scope_let_count == 0 - assert script.let is None - - def test_script_lets_only_is_zero(self): - script = self._script({"name": "S", "script": {"let": ["a = 1"], **_onrun("{{a}}")}}) - - assert script._template_scope_let_count == 0 - assert script.let == ["a = 1"] - - def test_step_lets_only(self): - script = self._script( - {"name": "S", "let": ["a = 1", "b = 2"], "script": _onrun("{{a}}{{b}}")} - ) - - assert script._template_scope_let_count == 2 - assert script.let == ["a = 1", "b = 2"] - - def test_both_scopes_counts_only_the_step_level_ones(self): - script = self._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"], @@ -285,126 +256,74 @@ def test_both_scopes_counts_only_the_step_level_ones(self): } ) - # THEN: the count is the step-level count, and the merged order puts the - # step-level bindings first — the two together are what makes a prefix - # skip correct. - assert script._template_scope_let_count == 2 - assert script.let == ["a = 1", "b = 2", "c = 3"] + # THEN + assert script.let == ["c = 3"] - def test_syntax_sugar_script_records_the_count(self): - # The de-sugaring path builds its own merged `let`, so it needs the same - # boundary recorded. - script = self._script( + @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"], - "python": {"script": "print(1)", "let": ["b = 2"]}, - }, - extensions=("EXPR", "FEATURE_BUNDLE_1"), + "let": ["a = 1", "b = 2"], + interpreter: {"script": "print(1)", "let": ["c = 3"]}, + } ) - assert script._template_scope_let_count == 1 - assert script.let == ["a = 1", "b = 2"] + # THEN + assert script.let == ["c = 3"] - def test_count_is_private_and_not_serialized(self): - # Hard requirement: recording the boundary must not change the model's - # serialized shape. - step = { - "name": "S", - "let": ["a = 1"], - "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, - } - job = create_job(job_template=_decode(_job([step])), job_parameter_values={}) - # WHEN - obj = model_to_object(model=job) +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.""" - # THEN - assert job.steps[0].script._template_scope_let_count == 1 - assert "_template_scope_let_count" not in json.dumps(obj) - assert obj["steps"][0]["script"]["let"] == ["a = 1", "c = 3"] - - @pytest.mark.parametrize("interpreter", ("bash", "python", "cmd", "powershell", "node")) - def test_syntax_sugar_records_the_count_without_a_step(self, interpreter): - # The worker agent parses a StepTemplate and calls resolve_syntax_sugar() - # directly, so no Step is built and the boundary has to be recorded at - # the de-sugaring merge — as the `script:` branch records it. - # GIVEN - sugar_script = self._resolved_script( - { - "name": "S", - "let": ["a = 1", "b = 2"], - interpreter: {"script": "print(1)", "let": ["c = 3"]}, - } - ) - script_branch = self._resolved_script( - { - "name": "S", - "let": ["a = 1", "b = 2"], - "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, - } - ) + @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}}")}, + } - # THEN: both merge the same list, so both must record the same boundary. - assert script_branch.let == ["a = 1", "b = 2", "c = 3"] - assert script_branch._template_scope_let_count == 2 - assert sugar_script.let == ["a = 1", "b = 2", "c = 3"] - assert sugar_script._template_scope_let_count == script_branch._template_scope_let_count - - @pytest.mark.parametrize( - "sibling_let", - (None, ["z = 9"]), - ids=("no-let", "non-matching-let"), - ) - def test_sibling_step_does_not_clear_a_shared_scripts_count(self, sibling_let): - # A merged script is a single object, and a Step keeps the instance it is - # given rather than revalidating it, so more than one Step can reach it. - # A sibling whose own `let` is empty or does not match the prefix - # computes 0 for itself; writing that over the marker the owning step - # recorded would revert the owner to re-evaluating template-scope - # bindings in host scope -- the bug the marker exists to prevent. + def test_step_let_values_are_carried_and_paths_render_per_host(self): # GIVEN - script = self._resolved_script( - { - "name": "S", - "let": ["a = 1", "b = 2"], - "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, - } - ) - owner = Step(name="S", script=script, let=["a = 1", "b = 2"]) - assert owner.script is script - assert script._template_scope_let_count == 2 + result = self._tables(self._STEP) + table = result.step_symbol_tables["S"] # WHEN - sibling = Step(name="T", script=script, let=sibling_let) - - # THEN - assert sibling.script is script - assert script._template_scope_let_count == 2 - - def test_step_records_its_own_count_and_an_unmarked_script_is_zero(self): - # Negative control for the "never lower the marker" rule: it must not - # stop a Step from recording the count its own `let` legitimately - # yields, and a script no Step has ever marked must read 0. + 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 - marked = self._resolved_script( - { - "name": "S", - "let": ["a = 1", "b = 2"], - "script": {"let": ["c = 3"], **_onrun("{{a}}{{c}}")}, - } - ) - unmarked = self._resolved_script({"name": "S", "script": _onrun("hi")}) - # The de-sugaring merge records on the merged script; strip it so this - # asserts the Step validator's own write, not the one already there. - fresh = marked.model_copy() - fresh._template_scope_let_count = 0 + result = self._tables(self._STEP) # WHEN - recording = Step(name="S", script=fresh, let=["a = 1", "b = 2"]) - never_marked = Step(name="U", script=unmarked) + symtab = result.step_symbol_tables["S"].to_symtab(path_format=PathFormat.POSIX) # THEN - assert recording.script._template_scope_let_count == 2 - assert unmarked.let is None - assert never_marked.script._template_scope_let_count == 0 + assert "scriptonly" not in symtab + assert "a" in symtab From af824c4b53920967802d5ca752239517d1086fbd Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:22:28 -0700 Subject: [PATCH 7/8] docs: Correct the extends_symtab ordering comment The comment justified running the symbol-table hook before the transform by saying StepTemplate's syntax-sugar transform folds step-level `let` bindings into the script. That fold was deleted in the previous commit, so the stated reason no longer exists -- but the ordering is still load-bearing, and this comment was its only record. Restate the two reasons that survive: * A transform may rebuild the model rather than adjust it -- `resolve_syntax_sugar` returns a `model_construct`ed StepTemplate -- so the fields the hook reads (`name`, `let`) are only guaranteed to be the authored ones on this side of it. The transform carries `let` through deliberately; running the hook first keeps that the transform's choice rather than a requirement on every future one. * `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. That table is only the scope the step's own fields were instantiated against if both callers hand the hook the same model. Comment only; no behaviour change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/model/_internal/_create_job.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) 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) From 66155af3ba6b8da22ca6d0df3a35b3be243d999a Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Sat, 29 Aug 2026 08:37:19 -0700 Subject: [PATCH 8/8] docs: Document the create_job step-let contract as breaking Dropping the step-level `let` merge means the Job returned by plain create_job() is no longer self-contained for an EXPR template whose step declares a `let` its script references: at 6a23815 script.let was ['a = 1'] and the task succeeded; at 260b147 script.let is None and the task fails with `Undefined variable: 'a'`. The removal is correct -- both channels evaluating caused the clobber this branch fixes -- so document the contract instead of adding a shim. Rewrite create_job's docstring to say plainly that the Job does not carry step-level `let` values and that a caller running such a job must use create_job_with_symbol_tables and forward the tables, add a CHANGELOG breaking-change entry, and add the same pointer to the README's "Creating a Job from a Job Template" example. The README's other two create_job() examples only inspect the Job at creation time (StepDependencyGraph, StepParameterSpaceIterator), so plain create_job stays correct there and they are left alone. BREAKING CHANGE: create_job() no longer returns a Job carrying evaluated step-level `let` 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. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- CHANGELOG.md | 14 ++++++++++++++ README.md | 9 +++++++++ src/openjd/model/_create_job.py | 20 ++++++++++++++++---- 3 files changed, 39 insertions(+), 4 deletions(-) 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/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,