Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)


Expand Down
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions THIRD-PARTY-LICENSES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
20 changes: 16 additions & 4 deletions src/openjd/model/_create_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
19 changes: 15 additions & 4 deletions src/openjd/model/_internal/_create_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
13 changes: 11 additions & 2 deletions src/openjd/model/_let_bindings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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}")
88 changes: 51 additions & 37 deletions src/openjd/model/v2023_09/_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -3447,16 +3448,32 @@ 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.
"""
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The POSIX pin only covers let bindings; the rest of create-time resolution still renders PATH values in the host format, so the host-independence property the docstring above states is not actually achieved for a step that puts a path expression somewhere other than a let.

The two create-time resolution entry points do not thread a path_format:

  • _internal/_create_job.py:283value.resolve(symtab=symtab) (no path_format), used for every resolve_fields field, e.g. HostRequirements name/min/max (line 3086) and parameterSpace range strings.
  • _internal/_create_job.py:48expression.evaluate_value(symtab=symtab) for RFC 0006 typed whole-field list resolution.

So a template with hostRequirements.amounts[].name: "{{ startswith(path(\"/foo/bar\"), \"/foo\") ? ... }}", or a task range built from a path expression, still evaluates against the creating host’s format and yields a different Job on Windows vs Linux — the exact failure the added test test_step_symtab_path_predicate_is_host_independent guards against, just reached through a field expression instead of a binding.

Worth either passing PathFormat.POSIX through those two call sites as well, or narrowing the _extend_step_symtab docstring to say only let bindings are pinned so the remaining gap is not read as closed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and open. Measured: create_job resolved min: '{{ 4 if startswith(path("/foo/bar"),"/foo") else 8 }}' to 4 through the host format, so every non-let template-scope field is still host-dependent, and openjd-rs pins POSIX at every create-time site where this PR pins one. The fix is to thread POSIX through _create_job.py:48 and :283, a behaviour change worth its own PR, so this stays open.

Comment thread
leongdl marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The POSIX guarantee stops at the let values themselves; the create-time consumers of those values still resolve in host format.

_extend_step_symtab now evaluates the bindings with PathFormat.POSIX, but the fields that consume them at job creation are resolved by instantiate_model -> _instantiate_noncollection_value, which calls value.resolve(symtab=symtab) with no path_format (src/openjd/model/_internal/_create_job.py:283). Those create-time-resolved fields include exactly the ones this docstring cites as the motivation: the step parameterSpace ranges (resolve_fields includes range, _model.py:1435) and hostRequirements (_model.py:3073, 3220).

So for a PATH-typed binding such as root = path("/foo/bar"), used from a task parameter range of "<<root>>/a,<<root>>/b" (double-brace interpolation), the binding is stored POSIX-correct as an ExprValue, but the range format string is rendered via ExprNode._evaluate_raw(path_format=None), so the engine coerces the path with the host separator. The instantiated Job then holds a backslash rendering when created on Windows and a slash rendering on Linux -- the same host-dependence the docstring says this change eliminates.

The new tests do not catch this because they only assert on bindings that coerce to a string inside the expression (string(path(...)), startswith(...)), where POSIX is already baked in at let-evaluation time.

If the intent is to match the openjd-rs hardcoded PathFormat::Posix for the whole of job instantiation, the format-string resolution during instantiation needs the same path_format threaded through it, not just the let evaluation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and a known open item that this PR does not close. Create-time consumers do still resolve in host format via _instantiate_noncollection_value's value.resolve(symtab=symtab) at _create_job.py:283, so parameterSpace ranges and hostRequirements remain host-dependent for a PATH-typed binding. It is tracked as its own change rather than folded in here, because threading POSIX through the whole of instantiation is a behaviour change that deserves its own PR and conformance run; leaving this thread open as the record.

)
return step_symtab

_template_variable_sources = {
Expand Down Expand Up @@ -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
Comment thread
leongdl marked this conversation as resolved.
# 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.
Comment thread
leongdl marked this conversation as resolved.
return self

for name, (command, ext, arg_prefix) in _INTERPRETER_MAP.items():
Expand All @@ -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,
Expand Down
59 changes: 59 additions & 0 deletions test/openjd/model_v0/test_let_bindings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Loading