Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
110 changes: 104 additions & 6 deletions src/openjd/sessions/_runner_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"NotifyCancelMethod",
"ScriptRunnerBase",
"apply_let_bindings",
"apply_script_let_bindings",
"resolve_action_arg_values",
"resolve_effective_cancelation",
"resolve_optional_int_field",
Expand Down Expand Up @@ -494,7 +495,9 @@ def resolve_period(period: Any) -> Optional[int]:
)


def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None:
def apply_let_bindings(
*, symtab: SymbolTable, let_bindings: list[str], path_format: Any = None
) -> None:
"""Evaluate EXPR ``let`` bindings (RFC 0005) and add them to ``symtab``.

``let_bindings`` is a script's ``let`` field: an ordered list of
Expand All @@ -510,6 +513,12 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None:
``Env.File.*``/``Task.File.*`` and a file's ``data`` may reference
let-bound values (mirroring openjd-rs's runner ordering).

``path_format`` is the EXPR ``PathFormat`` that PATH-typed results render
with. ``None`` -- the default, and what every session-scope binding wants --
leaves the engine's default, which is the host's format. Callers
re-evaluating a *template*-scope binding pass ``PathFormat.POSIX``; see
:func:`apply_script_let_bindings`.

Raises:
ValueError (FormatStringError/ExpressionError): if a binding's
expression cannot be evaluated, or if a binding is too long to
Expand All @@ -528,7 +537,84 @@ def apply_let_bindings(*, symtab: SymbolTable, let_bindings: list[str]) -> None:
)
# Single-sourced in openjd.model (parse-memoized; skips malformed
# bindings; raises ValueError naming the failing binding).
evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings)
#
# The kwarg is forwarded only when it is set, because `path_format` does not
# exist on openjd-model at this package's declared floor (>= 0.11.6) and
# passing it there is a TypeError, not a no-op -- which would break EVERY
# EXPR template rather than degrading. On such a model the else branch is
# unreachable: apply_script_let_bindings reads the template-scope count
# through getattr, and a model without `path_format` has no
# `_template_scope_let_count` either, so the count is 0 and nothing asks for
# a non-default format. On a model that does have it, `None` and "omitted"
# are the same call. Collapse this to an unconditional forward once the
# openjd-model floor carries the parameter.
if path_format is None:
evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings)
else:
evaluate_let_bindings(symtab=symtab, let_bindings=let_bindings, path_format=path_format)

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 graceful-degradation design here means the fix is a silent no-op on the declared dependency floor. pyproject.toml pins openjd-model >= 0.11.6, < 0.12; on 0.11.6 neither path_format nor _template_scope_let_count exists, so getattr(..., 0) returns 0, the split never happens, and the Windows PATH-rendering divergence this PR exists to fix is still live. An installation that resolves to the floor gets none of the fix and no signal that it did not.

If the model-side half has shipped, the floor should be raised to that version and this two-branch forward + getattr fallback can collapse to the unconditional form the comment already anticipates. If it has not shipped yet, consider a follow-up marker so the floor bump is not forgotten — as written there is nothing in the tree that will fail when the model catches up.

Relatedly, the new tests do not degrade the way the source does: test_template_scope_let_split.py::test_path_format_is_load_bearing and TestEndToEndScopeAgreement::test_a_step_level_path_binding_agrees_across_the_two_evaluations call apply_let_bindings(..., path_format=...) directly, which raises TypeError on a floor-version model. So the suite already assumes a model newer than the declared floor.

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 blocked rather than fixable here: released openjd-model 0.11.6 has neither path_format nor _template_scope_let_count, so on the declared floor the split never happens. The floor gets raised to the release carrying openjd-model#341, which is step 4 of the merge order and also clears the known mypy failure. Staying open until that releases.



def apply_script_let_bindings(
Comment thread
leongdl marked this conversation as resolved.
Outdated
*, symtab: SymbolTable, let_bindings: list[str], script: Any = None
) -> None:
"""Evaluate a script's ``let`` list into ``symtab``, honouring the
template-scope / session-scope boundary inside it.

An instantiated Step's script carries a *merged* ``let`` list: the
step-level bindings the template declared, followed by the script's own
(openjd-model's ``StepTemplate.resolve_syntax_sugar``). The step-level
prefix was already evaluated at job creation, in **template** scope, which
openjd-rs -- and now openjd-model -- evaluate with ``PathFormat::Posix`` so
that a create-time result cannot depend on the host that created the job.
Re-evaluating that prefix here in the host's format re-renders its PATH
values: on Windows ``path("/foo/bar")`` becomes ``\\foo\\bar``, so
``startswith(path("/foo/bar"), "/foo")`` flips from ``true`` to ``false``
and the binding's value silently differs between the two evaluations.

So the list is split at the boundary and the two halves are evaluated in
different path formats -- the prefix as POSIX, the remainder (the script's
own bindings) with the host's format, unchanged. Script-level bindings
legitimately see host-scope symbols (``Session.WorkingDirectory``,
``Task.File.*``, ``apply_path_mapping``), so their format must stay the
host's.

Both halves are evaluated into the **same** table in the **same** order,
because a later binding may reference an earlier one -- including a
script-level binding referencing a step-level one.

``script`` is the model object the ``let`` list came from. The boundary is
read off it as ``_template_scope_let_count``, through ``getattr`` with a
default of 0: an openjd-model that predates the model-side half of this fix
does not carry the attribute, and must degrade to exactly the previous
behaviour (everything in host format) rather than raising. ``None`` -- what
an environment script's caller passes -- means the same thing: an
environment script's own bindings are session scope and correctly use the
host format.

Raises:
ValueError: as :func:`apply_let_bindings`.
"""
# min() because the count comes from a separate distribution: a model/
# sessions version skew that reported a longer prefix than the list would
# otherwise silently evaluate nothing at all here.
template_scope_count = min(getattr(script, "_template_scope_let_count", 0), len(let_bindings))
Comment thread
leongdl marked this conversation as resolved.
Outdated
if template_scope_count:
# Lazy AND conditional (see the module comment on _EXTENSION_MODULE): a
# function-local import still fires unconditionally once its function is
# called, so it sits behind "there is a template-scope prefix to
# evaluate". Only a step script with step-level `let` bindings reaches
# here, and a `let` field only parses under the EXPR extension -- which
# has already loaded the extension. A non-EXPR session never gets here.
from openjd.expr import PathFormat

apply_let_bindings(
symtab=symtab,
let_bindings=let_bindings[:template_scope_count],
path_format=PathFormat.POSIX,
)
host_scope_bindings = let_bindings[template_scope_count:]
if host_scope_bindings:
apply_let_bindings(symtab=symtab, let_bindings=host_scope_bindings)
Comment thread
leongdl marked this conversation as resolved.
Outdated


class ScriptRunnerBase(ABC):
Expand Down Expand Up @@ -1020,10 +1106,17 @@ def _materialize_files(
symtab: SymbolTable,
let_bindings: Optional[list[str]] = None,
preallocated_records: Optional[list[_FileRecord]] = None,
script: Any = None,
) -> None:
"""Helper for derived classes that wraps all of the logic around
materializing embedded files to disk.

``script`` is the model object ``let_bindings`` came from, forwarded to
:func:`apply_script_let_bindings` so a step script's merged list is
split at its template-scope boundary. Omitting it evaluates every
binding in the host's path format, which is what an environment script
wants.

When ``let_bindings`` is given, they are evaluated between file-path
allocation and content writing (RFC 0005, mirroring the openjd-rs
runners): a file's *path* never depends on ``let`` values (filenames
Expand Down Expand Up @@ -1061,20 +1154,25 @@ def _materialize_files(
else:
records = file_writer.allocate_file_paths(files, symtab)
if let_bindings:
apply_let_bindings(symtab=symtab, let_bindings=let_bindings)
apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script)
file_writer.write_file_contents(records, symtab)
except (RuntimeError, ValueError) as exc:
# Had a problem writing at least one file to disk, or evaluating
# a `let` binding (FormatStringError/ExpressionError subclass
# ValueError). Surface the error.
self._fail_action(str(exc))

def _apply_let_bindings_or_fail(self, symtab: SymbolTable, let_bindings: list[str]) -> bool:
def _apply_let_bindings_or_fail(
self, symtab: SymbolTable, let_bindings: list[str], script: Any = None
) -> bool:
"""Evaluate the script's EXPR ``let`` bindings into ``symtab``. On an
evaluation error the action is failed through the normal failure path
(openjd_fail log, FAILED state, callback). Returns True on success."""
(openjd_fail log, FAILED state, callback). Returns True on success.

``script`` is the model object the bindings came from; see
:func:`apply_script_let_bindings` for what it is read for."""
try:
apply_let_bindings(symtab=symtab, let_bindings=let_bindings)
apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script)
except ValueError as exc:
self._fail_action(str(exc))
return False
Expand Down
7 changes: 6 additions & 1 deletion src/openjd/sessions/_runner_step_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ def run(self) -> None:
# the script's EXPR `let` bindings evaluate (so bindings can reference
# Task.File.*), and contents are written after (so `data` can
# reference let-bound values) — mirroring the openjd-rs runner.
#
# `script=self._script` is what tells the evaluation where this merged
# `let` list stops being template scope and starts being session scope;
# see apply_script_let_bindings.
if self._script.embeddedFiles is not None:
symtab = SymbolTable(source=self._symtab)
self._materialize_files(
Expand All @@ -110,12 +114,13 @@ def run(self) -> None:
self._session_files_directory,
symtab,
let_bindings=let_bindings,
script=self._script,
)
if self.state == ScriptRunnerState.FAILED:
return
elif let_bindings:
symtab = SymbolTable(source=self._symtab)
if not self._apply_let_bindings_or_fail(symtab, let_bindings):
if not self._apply_let_bindings_or_fail(symtab, let_bindings, self._script):
return
else:
symtab = self._symtab
Expand Down
19 changes: 16 additions & 3 deletions src/openjd/sessions/_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
from ._path_mapping import PathMappingRule
from ._runner_base import (
ScriptRunnerBase,
apply_let_bindings,
apply_script_let_bindings,
resolve_action_arg_values,
resolve_effective_cancelation,
resolve_optional_int_field,
Expand Down Expand Up @@ -1998,6 +1998,7 @@ def _build_wrapped_inner_scope(
let_bindings: Optional[list[str]],
embedded_files: Optional[Any],
base: SymbolTable,
script: Any = None,
) -> SymbolTable:
"""Build the scope a wrapped action would have resolved against had
it run unwrapped: a copy of ``base`` (the session-scope table) plus
Expand All @@ -2012,6 +2013,17 @@ def _build_wrapped_inner_scope(
symmetrically, the inner entity's lets never apply to the hook's own
resolution scope. Mirrors openjd-rs's ``build_wrapped_inner_scope``.

``script`` is the inner entity's script -- the model object
``let_bindings`` came from -- forwarded so that a wrapped *step* script's
merged ``let`` list is split at its template-scope boundary exactly as
the step runner splits it (see
:func:`~._runner_base.apply_script_let_bindings`). Without it a wrapped
action would resolve against template-scope values re-rendered in the
host's path format, i.e. against a scope that differs from the one it
would have had unwrapped -- which is the whole property this method
exists to reproduce. An inner *environment* script has no such prefix and
is unaffected.

Raises:
ValueError (FormatStringError/ExpressionError): a binding or file
reference did not resolve.
Expand All @@ -2027,10 +2039,10 @@ def _build_wrapped_inner_scope(
)
records = file_writer.allocate_file_paths(embedded_files, symtab)
if let_bindings:
apply_let_bindings(symtab=symtab, let_bindings=let_bindings)
apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script)
Comment thread
leongdl marked this conversation as resolved.
Outdated
file_writer.write_file_contents(records, symtab)
elif let_bindings:
apply_let_bindings(symtab=symtab, let_bindings=let_bindings)
apply_script_let_bindings(symtab=symtab, let_bindings=let_bindings, script=script)
return symtab

def _try_inject_wrapped_symbols(
Expand Down Expand Up @@ -2061,6 +2073,7 @@ def _try_inject_wrapped_symbols(
inner_script.let if inner_script is not None else None,
inner_script.embeddedFiles if inner_script is not None else None,
symtab,
inner_script,
)
inject(inner_symtab)
except (FormatStringError, ValueError, RuntimeError) as e:
Expand Down
Loading
Loading