-
Notifications
You must be signed in to change notification settings - Fork 23
fix: Evaluate step-level let bindings in template scope #341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mainline
Are you sure you want to change the base?
Changes from all commits
785b48b
48e91a3
30dd59d
82af064
6a23815
260b147
af824c4
66155af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3388,8 +3388,9 @@ class Step(OpenJDModel_v2023_09): | |
| # RFC 0007 (EXPR): the step-level `let` bindings, preserved from the | ||
| # StepTemplate so the runtime can seed them when entering the step's | ||
| # environments — a step environment's variables and actions may reference | ||
| # them. The step's own script carries a merged copy (step bindings first) | ||
| # for the task-run path. | ||
| # them. Their *values* are already resolved at job creation and travel in | ||
| # the step's symbol table (see create_job_with_symbol_tables), so they are | ||
| # not merged into the script's own `let` for the runtime to re-evaluate. | ||
| let: Optional[list[str]] = None | ||
|
|
||
|
|
||
|
|
@@ -3447,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 | ||
|
leongdl marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The POSIX guarantee stops at the
So for a PATH-typed binding such as The new tests do not catch this because they only assert on bindings that coerce to a string inside the expression ( If the intent is to match the openjd-rs hardcoded
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| ) | ||
| return step_symtab | ||
|
|
||
| _template_variable_sources = { | ||
|
|
@@ -3573,17 +3590,13 @@ def resolve_syntax_sugar(self) -> "StepTemplate": | |
| StepTemplate: A new StepTemplate with de-sugared script, or self if no sugar. | ||
| """ | ||
| if self.script: | ||
| # Step-level `let` (RFC 0007) is excluded from the instantiated Step | ||
| # by the job-creation metadata, so fold it into the script's own | ||
| # `let` (step bindings first, then the script's) so it survives into | ||
| # the Job and the runtime resolves it. The model has already | ||
| # validated reference/shadowing rules across both scopes at decode. | ||
| if self.let: | ||
| # The step's own `let` is preserved too (Step.let): the | ||
| # runtime seeds it when entering the step's environments. | ||
| merged_let = [*self.let, *(self.script.let or [])] | ||
| new_script = self.script.model_copy(update={"let": merged_let}) | ||
| return self.model_copy(update={"script": new_script}) | ||
| # The step-level `let` (RFC 0007) is *not* folded into the script's | ||
|
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. | ||
|
leongdl marked this conversation as resolved.
|
||
| return self | ||
|
|
||
| for name, (command, ext, arg_prefix) in _INTERPRETER_MAP.items(): | ||
|
|
@@ -3608,32 +3621,33 @@ def resolve_syntax_sugar(self) -> "StepTemplate": | |
| args.extend(simple_action.args) | ||
|
|
||
| # Construct directly - inputs are already validated | ||
| new_script = StepScript.model_construct( | ||
| actions=StepActions.model_construct( | ||
| onRun=Action.model_construct( | ||
| command=CommandString(command), | ||
| args=args, | ||
| timeout=simple_action.timeout, | ||
| cancelation=simple_action.cancelation, | ||
| ) | ||
| ), | ||
| # Only the SimpleAction's own `let` (RFC 0007) — the step-level one | ||
| # is resolved at job creation and travels in the step's symbol | ||
| # table, as in the `script:` branch above. | ||
| let=simple_action.let, | ||
| embeddedFiles=[ | ||
| EmbeddedFileText.model_construct( | ||
| name=embedded_name, | ||
| type=EmbeddedFileTypes.TEXT, | ||
| filename=f"{embedded_name}{ext}", | ||
| runnable=True, | ||
| data=simple_action.script, | ||
| ) | ||
| ], | ||
| ) | ||
| return StepTemplate.model_construct( | ||
| name=self.name, | ||
| description=self.description, | ||
| script=StepScript.model_construct( | ||
| actions=StepActions.model_construct( | ||
| onRun=Action.model_construct( | ||
| command=CommandString(command), | ||
| args=args, | ||
| timeout=simple_action.timeout, | ||
| cancelation=simple_action.cancelation, | ||
| ) | ||
| ), | ||
| # Carry step-level `let` (RFC 0007) and the SimpleAction's own | ||
| # `let` onto the de-sugared script (step bindings first) so they | ||
| # are preserved into the Job and resolved at runtime. | ||
| let=([*(self.let or []), *(simple_action.let or [])] or None), | ||
| embeddedFiles=[ | ||
| EmbeddedFileText.model_construct( | ||
| name=embedded_name, | ||
| type=EmbeddedFileTypes.TEXT, | ||
| filename=f"{embedded_name}{ext}", | ||
| runnable=True, | ||
| data=simple_action.script, | ||
| ) | ||
| ], | ||
| ), | ||
| script=new_script, | ||
| stepEnvironments=self.stepEnvironments, | ||
| parameterSpace=self.parameterSpace, | ||
| hostRequirements=self.hostRequirements, | ||
|
|
||
There was a problem hiding this comment.
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
letbindings; 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 alet.The two create-time resolution entry points do not thread a
path_format:_internal/_create_job.py:283—value.resolve(symtab=symtab)(nopath_format), used for everyresolve_fieldsfield, e.g.HostRequirementsname/min/max(line 3086) andparameterSpacerange strings._internal/_create_job.py:48—expression.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 testtest_step_symtab_path_predicate_is_host_independentguards against, just reached through a field expression instead of a binding.Worth either passing
PathFormat.POSIXthrough those two call sites as well, or narrowing the_extend_step_symtabdocstring to say onlyletbindings are pinned so the remaining gap is not read as closed.There was a problem hiding this comment.
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_jobresolvedmin: '{{ 4 if startswith(path("/foo/bar"),"/foo") else 8 }}'to 4 through the host format, so every non-lettemplate-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:48and:283, a behaviour change worth its own PR, so this stays open.