fix: Re-evaluate a step's template-scope let bindings in template scope - #362
fix: Re-evaluate a step's template-scope let bindings in template scope#362leongdl wants to merge 11 commits into
Conversation
An instantiated Step's `script.let` is a merged list: the step-level bindings
the template declared, followed by the script's own. The step-level prefix was
already evaluated at job creation in template scope, which openjd-rs — and
openjd-model as of the companion change — evaluate with `PathFormat::Posix` so
that a create-time result cannot depend on the host that created the job. The
runners re-evaluated the whole merged list in the host's format, so on Windows
the prefix's PATH values were re-rendered with backslashes and a binding
silently held a different value in the two evaluations:
`startswith(path("/foo/bar"), "/foo")` flipped from true to false.
`apply_script_let_bindings` now owns the split. The leading
`_template_scope_let_count` entries are evaluated with `PathFormat.POSIX` and
the remainder with the host's format, unchanged — a script's own bindings are
session scope and legitimately reference `Session.WorkingDirectory`,
`Task.File.*` and `apply_path_mapping`. Both halves go into the same symbol
table in the same order, so a script-level binding can still reference a
step-level one.
Applied at all three sites that evaluate a step script's merged list: the step
runner, the step runner's embedded-files path, and RFC 0008's
`_build_wrapped_inner_scope`. Environment scripts are untouched.
The count is read with `getattr(script, "_template_scope_let_count", 0)`, so an
openjd-model without the companion change degrades to the previous behaviour.
`path_format` is likewise forwarded to `evaluate_let_bindings` only when set,
because the parameter does not exist at the currently declared openjd-model
floor and passing it there raises TypeError rather than being ignored.
This is the openjd-sessions half of a two-repo fix; see
OpenJobDescription/openjd-model-for-python#341. The openjd-model floor in
pyproject.toml must be raised once that half releases, and until then
`hatch run typing` reports one error against the PyPI model.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The previous commit evaluated the template-scope prefix with
`PathFormat.POSIX` and seeded the result directly. That is wrong, and on
Windows it is worse than the bug it replaced.
An EXPR path value carries its format, and reading one under a different format
is a hard error rather than a re-render:
ExpressionError: Path format mismatch for 'root':
value has Posix but evaluator uses Windows
Action arguments, embedded-file `data` and environment-variable values all
resolve in the host's format, so a Posix-tagged path seeded into the session
table made every one of those reads raise. That would have broken
`EXPR/jobs/expr2.3.2--path-construction`, a conformance fixture that passes
today and asserts `STR:\a\b` on Windows.
The conformance suite states the rule precisely. A binding whose result leaves
path-space must freeze the text template scope produced, so
`expr2.2.1--string-conversion` wants `/mnt/out` on both platforms. A binding
whose result is still a path must render in the host's format, so
`expr2.3.2--path-construction` wants `\a\b` on Windows.
So `_apply_template_scope_let_bindings` now evaluates the prefix into a child
table with POSIX, then re-tags the results to the host's format through a
`SerializedSymbolTable` round trip before seeding them. That leaves a frozen
string alone and re-renders a live path, which is both halves of the rule. It
mirrors how a create-time table already reaches a session, via
`Session._resolved_base_entries` and its `to_symtab(path_format=host_format)`.
Verified with the host format forced to Windows: a step-level `path('/a/b')`
reads back as `\a\b`, `string(path('/mnt/out'))` stays `/mnt/out`,
`startswith(path('/foo/bar'), '/foo')` stays `true`, and a session-scope
binding is untouched.
Also from review: an out-of-range boundary now falls back to session scope
instead of being clamped into range. Clamping would evaluate a genuinely
session-scope binding in template scope; the fallback is the pre-fix behaviour,
which never mis-scopes. Two comments that over-claimed are corrected, one about
this module having a single crossing into `openjd.expr` and one about a branch
being unreachable when this PR's own tests reach it.
Suite: 1012 passed, 0 failed, 40 skipped, 16 xfailed.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
|
Pushed a correction. A local three-reviewer pass found that the first version of this change was wrong on Windows, worse than the bug it replaced, so please review from An EXPR path value carries its format, and reading one under a different format raises rather than re-rendering: Action args, embedded-file The conformance suite states the rule: a binding whose result leaves path-space must freeze the text template scope produced ( Companion correction in openjd-model-for-python#341 at Still unverified: Windows itself. This PR's Windows legs have not run to completion, and its CI stays red until #341 releases and the |
The end-to-end test used count=1, leaving the second binding in session scope. On a Windows host that binding reads its path neighbour in host format and evaluates to false, which is the designed behaviour, so the test asserted true for a value that is false there. It passed on POSIX and would have failed Windows CI the first time those legs ran. Both step-level bindings belong in the prefix, which is what the test is about. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| """ | ||
| from openjd.expr import PathFormat, SerializedSymbolTable | ||
|
|
||
| from openjd.model._format_strings._expr_support import symtab_to_expr_values |
There was a problem hiding this comment.
This reaches two levels into openjd-model's private API — openjd.model.**_format_strings**.**_expr_support**.symtab_to_expr_values — and it is the only such import in openjd/sessions/ (the two in _v1/ are openjd.model._v1.*, a versioned package, not an internal implementation module). Nothing about _format_strings._expr_support is covered by openjd-model's compatibility surface, so a rename or relocation in any 0.11.x patch breaks it.
The failure mode is what makes this more than a style point: an ImportError here is raised from inside apply_script_let_bindings, and neither caller catches it. _apply_let_bindings_or_fail catches ValueError only (line 1234), _materialize_files catches (RuntimeError, ValueError) (line 1216), and _try_inject_wrapped_symbols catches (FormatStringError, ValueError, RuntimeError) (_session.py:2077). So an openjd-model that moved this function turns every EXPR step-with-step-level-lets into an unhandled ImportError out of the public Session.run_task — not a failed action, and not the graceful degradation the rest of this function is carefully built around (getattr for the count, the two-branch forward for path_format).
Also worth noting the pinning gap: pyproject.toml:42 documents what the >= 0.11.6 floor is for by name (CancelationMethodDeferred, SymbolTable.expr_host_rules, ...). This PR adds three new model/engine requirements — evaluate_let_bindings(path_format=), symtab_to_expr_values, SerializedSymbolTable.from_symtab — and updates neither the floor nor that comment.
If openjd-model has (or could add) a public equivalent, using it would remove the coupling entirely. Failing that, catching ImportError alongside the range guard and degrading to the unsplit path would at least keep the "never raises out of the public API" property this function otherwise maintains.
There was a problem hiding this comment.
Correct and open: the floor gets raised to the release carrying openjd-model#341, which also clears the known mypy failure, and an ImportError guard goes in next to the range guard. Blocked on that release, so this stays open.
| """ | ||
| import os | ||
|
|
||
| from openjd.expr import PathFormat |
There was a problem hiding this comment.
The updated _classify_expr_value docstring now says "one of two places in this module that import openjd.expr" and names _apply_template_scope_let_bindings as "the other crossing" — but this PR adds three: line 190, line 602, and this one. _host_path_format is a third, and it is the only one that is a standalone module-level function with no guard of its own, so the "structural instead of merely documented" property that paragraph is defending is now weaker than the paragraph claims.
Concretely: any future caller of _host_path_format() gets an unguarded import openjd.expr — which is exactly the class of regression test_import_purity.py exists to catch (per its module docstring, a load-time dependency on the native extension breaks a consumer that only runs non-EXPR templates). Today the only caller is inside the prefix guard, so the purity tests pass; nothing stops that from changing, and the docstring now reads as if it had been audited.
Two small things that would restore the invariant:
- Either fold the
PathFormatlookup into_apply_template_scope_let_bindings(which already importsPathFormaton line 602, so the helper adds no reuse), or update the docstring to say three and note that this one is guarded only by its single call site. import oson line 573 shadows the module-levelosalready imported at line 4. Harmless but it reads as ifoswere unavailable at module scope, which would mislead a future reader about why the import is local (theopenjd.exprimport is the one that must be deferred;osis not).
There was a problem hiding this comment.
Correct: there are three crossings at this head, 190, 565 and 602, not two, and import os at 573 also shadows line 3. Bundled with the floor bump as one tidy-up and not in at this head, so leaving the thread open.
The prefix was evaluated with PathFormat.POSIX against a child of the session symbol table, which holds host-format values. On a Windows host that either raises `Path format mismatch` for a PATH job parameter or a natively seeded create-time value, or silently succeeds against a re-rendered one -- `.parent` of a Windows path read as POSIX is '.', because a backslash is an ordinary POSIX path character. The previous commit fixed the output side and left this input side broken. The prefix now evaluates against only the symbols in scope that carry no path format, plus the prefix bindings already bound. The filter tests shape rather than name, because the set of session symbols grows and a name denylist would rot: a symbol is excluded if its value is a native path-typed engine value, or its expr_types entry declares PATH or LIST[PATH]. That matches the measured blast radius exactly. A prefix binding that needs an excluded symbol now fails with `Undefined variable`, and the whole let list falls back to one host-format evaluation. All-or-nothing on purpose: freezing per binding would leave a POSIX-evaluated binding reading a host-evaluated sibling, which is the same cross-format read somewhere less visible. The fallback is the pre-fix behaviour exactly, so it cannot regress, and a genuine evaluation error still surfaces from it with the same message. This makes the change a smaller claim: it freezes self-contained template-scope bindings and declines the rest. Reproducing the rest needs the create-time value carried to the session rather than recomputed. 9 tests added, each mutation-checked against a revert of the production change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Four assertions in this file compared a rendered value against a POSIX
literal. Three of them are wrong on Windows, not merely weaker: a binding
still holding a path is seeded in the host's format, so
`path('/foo/bar')` reads `\foo\bar` there and both texts are correct. The
Windows leg was red because of them, and fail-fast cancelled it before it
reported, so these assertions had never been judged on Windows at all.
The rule they now state: a binding whose result has left path-space
freezes the POSIX text it froze at job creation; a binding still holding
a path renders in the host's format.
`_as_the_host_renders` builds the expectation by evaluating the same
expression through the same machinery at the engine default -- which is
the host's format -- rather than hardcoding one literal per platform
behind a conditional. It reaches the answer by a different route than the
code under test, a direct host-format evaluation rather than a POSIX
evaluation re-tagged, so it is not asserting the code against itself.
Fixed:
- `root` and `under` in test_ordering_is_preserved_across_the_boundary.
`under` is session scope and reads `root` in host format, so `false` on
Windows is correct behaviour, not a defect.
- `inner["tmpl"]` in the RFC 0008 wrapped-inner-scope test.
- The end-to-end agreement test, where the *fixture* was at fault:
`session_time` is re-tagged to the host and `create_time` came from a
bare POSIX `apply_let_bindings` with no re-tag, so the two sides sat at
different points of the journey a create-time table takes to reach a
session. Both sides are now read at the same point, and the test still
makes its point that they agree.
Verified on both legs, 25 passed each: unpatched, and with the host path
format and the engine default both forced to WINDOWS in-process. Three
production mutants confirm the corrected assertions pin behaviour --
dropping the re-tag is caught by all three on the Windows leg and by none
on POSIX, which is precisely the coverage this commit restores.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
`_apply_template_scope_let_bindings` copied `symtab.expr_host_rules` into the scratch table it evaluates the template-scope prefix against. The copy was inert for every valid binding and the comment defending it was wrong. `apply_path_mapping` is the only host-context function, and RFC 0005 bars those from template scope: openjd-model invokes the create-time hook with no host context, so a template-scope binding calling one raises `Unknown function: 'apply_path_mapping'` at job creation and never had a create-time value to reproduce. Measured separately, `expr_host_rules` of `None` versus `[]` render `path().parent`, arithmetic and `join` identically, so the copy changed nothing for a conforming template. For a template that does violate RFC 0005 the copy was actively harmful: it let the binding resolve here against the *host's* rules while evaluating in POSIX, freezing a mixed-separator value such as `C:\Users\test/bar` instead of declining. Dropping the copy makes such a binding raise `Unknown function`, which the existing `except ValueError` turns into the whole-list host-format fallback -- the documented behaviour for a prefix that cannot be reproduced in template scope, and the pre-fix value. Two tests. The first pins the fallback (not the raise, which is internal) for a prefix binding calling `apply_path_mapping`. The second guards the derived-table shape the runners actually pass, `SymbolTable(source=...)`, and pins openjd-model's `_expr_types` copy in `SymbolTable.__init__`: if that is ever dropped, `declared_types` goes empty and the PATH filter silently stops filtering. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
openjd-model no longer merges a step's template-scope `let` into `script.let`. The step's bindings are resolved once at job creation and travel in the step symbol table, which reaches a session through the existing `resolved_symtab` parameter and `_resolved_base_entries`, so `script.let` now holds only the script's own bindings. Those are genuinely session scope and correctly evaluate in the host's format, which is what they did before this branch. That makes the prefix-splitting apparatus redundant, and worse than redundant: with both mechanisms active the session-side re-evaluation wrote last and clobbered the correctly-formatted seeded value. Remove `_apply_template_scope_let_bindings`, `_is_format_neutral`, `_type_carries_path_format`, `_host_path_format`, every read of `_template_scope_let_count`, the caller logic that split the list, the all-or-nothing fallback, and the private `openjd.model._format_strings._expr_support` import that existed only for this feature. `apply_script_let_bindings` is now a single host-format evaluation of the whole list. This also drops the last use of the `path_format` keyword argument to `evaluate_let_bindings`, which does not exist on the released openjd-model 0.11.6 that CI resolves from PyPI. `mypy src test` against 0.11.6 is now clean, so this branch no longer waits on the openjd-model change releasing. Replace the 27 tests of the deleted design, plus the import-purity positive control that pinned its guarded `PathFormat` import, with four tests of the behaviour that matters: a seeded create-time path binding survives a script's own `let`, a step-level binding is never evaluated at session time, and a script's own `let` still evaluates in the host's format and still sees `Session.WorkingDirectory`. The Windows simulation patches both seams that choose a path format, because patching only the host-format derivation leaves the engine default at POSIX and yields a host that cannot exist. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
`apply_script_let_bindings` took a `script` model object that nothing read. It was threaded there through `ScriptRunnerBase._materialize_files`, `ScriptRunnerBase._apply_let_bindings_or_fail` and `Session._build_wrapped_inner_scope`, each of which carried the parameter only to forward it, and each of which documented in a paragraph that it was unused. The parameter was live when it was introduced in dbfd1b5: a step script's `let` list was then a merge of template scope and session scope, and the script was read to find the boundary between them. 5c3c8c9 removed that re-evaluation, so a script's `let` is now entirely its own scope and needs no per-script information to evaluate. An unused parameter plus a paragraph explaining that it is unused is worse than neither. No behaviour change: the parameter had no reader on any path. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The host-format test normalised path separators on only one side, so on
Windows CI, where both sides already render with backslashes, the
normalised left side no longer matched:
assert 'C:/ProgramData/Amazon/OpenJD/60htba6m'
== 'C:\ProgramData\Amazon\OpenJD\60htba6m'
`session.working_directory` is a real path object in the host OS's
flavour, while the binding renders in the format `_windows_host` forces.
Compare both sides through `PureWindowsPath` instead: the claim is
*which* path the binding saw, not how it renders, and the format claim
is the neighbouring `built` assertion. Reproduced on Windows 3.11, 3.12
and 3.14.
Also make the `apply_script_let_bindings` purity probe say something. It
used a malformed binding, which openjd-model skips without parsing, so
the evaluation path never ran and the assertion was near-tautological.
Split it in two: an empty `let` list must not load the native extension,
which is the production-reachable purity claim, and a valid non-path
binding (`mine = 1 + 1`, with its bound value asserted so a silent skip
fails) does load it. The latter is recorded as a documented limitation
rather than forced green, alongside
`test_path_mapping_rules_do_load_the_extension`.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
…ocstrings apply_script_let_bindings was a pure pass-through to apply_let_bindings, both public in __all__ with identical runtime behaviour. apply_let_bindings was already public on mainline and this branch added the wrapper, so delete the wrapper and keep the scope documentation -- the valuable part -- on apply_let_bindings. Repoints the four production call sites (_session.py x2, _runner_base.py x2) plus the test and comment references. Also corrects two claims in test_let_binding_scopes.py that measurement did not support: - The module docstring said TestSeededStepValuesAreNotReEvaluated "is what fails if anyone reintroduces session-side re-evaluation". It does not: it builds the script's `let` list itself, so it cannot observe a re-merge. What it does pin is host-format deserialization of resolved_symtab -- forcing _session.py's host_format to POSIX fails it. The re-merge half is pinned model-side by TestStepLetIsNotMergedIntoScript, whose six cases all fail against the pre-fix _model.py. The docstring now says both. - _windows_host's mock_patch of openjd.sessions._session.os.name is process-wide, not module-scoped, because _session.py does `import os` (openjd.sessions._session.os is os). It is inert today -- os.name is read exactly once in _session.py, at the intended seam -- and a module-scoped patch is unavailable without changing that import, so the global scope is now noted for whoever next adds a call inside the block. No behaviour change. Suite unchanged at 1000 passed, 40 skipped, 16 xfailed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The section comment and docstring said reaching apply_let_bindings with an empty let list was "the session path for every script, EXPR or not" and a "production-reachable purity claim". Neither holds: every call site guards on truthiness first (_session.py, _runner_base._materialize_files, and both callers of _apply_let_bindings_or_fail), so let_bindings=[] is a test-only shape. The docstring's clause about the binding-length guard was also vacuous, since that loop body never runs on an empty list. Reword to what the test does pin: openjd-model's evaluate_let_bindings staying pure on an empty list, as a dependency-boundary control. Point at the two tests that already cover production purity for a non-EXPR script. Text only, no behaviour change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
1 participant
What changed
A session now evaluates exactly one
letscope: the script's own, in the host's path format. It no longer re-evaluates a step's template-scopelet.A step's template-scope
let(RFC 0007 §3.6) is resolved once at job creation withPathFormat.POSIX, so a create-time value cannot depend on the host that created the job. openjd-model used to also merge those bindings intoscript.let, and the runners re-evaluated the merged list at session time in the host's format, re-rendering its PATH values. On Windows,startswith(path("/foo/bar"), "/foo")flips from true to false between the two evaluations.The re-evaluation was not a harmless second opinion. The seeded create-time value and the re-evaluated one land in the same symbol table, and the re-evaluation writes last, so it overwrote the correct value.
The fix is to delete the re-evaluation and read the resolved table that already arrives through
Step.resolved_symtab. That removed 268 lines from_runner_base.pyrelative to this branch's high-water mark: the template-scope/session-scope split, the format-neutral symbol filter that let a POSIX evaluation run against a host-format symbol table, the output-side re-tag, the boundary-marker plumbing, and the all-or-nothing fallback. None of it is needed once nothing re-evaluates. The final commit also drops ascriptparameter that had no reader left.This is what openjd-rs has always done, and what the Deadline worker agent adopted in public commit
08a5878b, "feat: forward the resolved symbol table to the v0 session", where it replacedextra_let_bindingsoutright. A convergent fix that adopts an existing upstream design, not a new one.This PR no longer depends on #341 releasing
It did, and that is worth stating plainly because the earlier description said the opposite. Nothing in
src/passespath_formattoevaluate_let_bindingsany more, so this branch no longer calls an API that exists only on an unreleased openjd-model.Verified in a virtual environment pinned to the released
openjd-model==0.11.6:mypy srcreports no issues across 40 source files. On that same released model,evaluate_let_bindingshas nopath_formatparameter at all, which is what made the dependency real before and what makes its absence checkable now.The companion model PR, OpenJobDescription/openjd-model-for-python#341, is still the other half of the behaviour — without it a step's bindings are still merged into
script.letand this PR simply evaluates that merged list in the host's format, as today. But the two can now release in either order.Migration
Removing the merge in the model half is a breaking change for any consumer that calls
resolve_syntax_sugar()and does not forward a resolved symbol table to its session. Such a consumer silently loses step-level bindings rather than failing. The worker agent forwards; openjd-cli now does too, in OpenJobDescription/openjd-cli#237. A third-party consumer would not, and there is no in-process fallback, because no PythonStepcarries the resolved table.Verification
Suite: 999 passed, 40 skipped, 16 xfailed, 0 failed.
Conformance through the branch CLI: 1172 passed, 2 failed.
expr2.2.1--string-conversionandexpr2.3.2--path-constructionboth pass — the two fixtures that pull in opposite directions, one wanting a value that has left path-space to stay/mnt/outand the other wanting a path to render in the host's format. The 2 failures are pre-existing range-normalization cases, unrelated to this change and blocked on a spec ruling.Other suites: model 5496, cli 318.
ruff checkandblack --checkclean at the CI-pinned versions (ruff 0.15, black 26).Not verified
Windows. This PR's Windows CI legs have been fail-fast cancelled on every round, so Windows has never been exercised by CI here. The behaviour was proven by simulation instead: with the host format forced to Windows in-process, a path-typed binding renders
\a\bwhile a value that has left path-space stays/mnt/out.The conformance runner concatenates a fixture's
outputblock with itsoutput_<os>block, so on a POSIX host anoutput_windowsblock is never asserted. A green POSIX conformance run proves nothing about the Windows expectations.