Skip to content

feat: deliver step-scope let bindings to run_task - #356

Closed
leongdl wants to merge 1 commit into
OpenJobDescription:mainlinefrom
leongdl:feat/step-scope-let-run-task
Closed

feat: deliver step-scope let bindings to run_task#356
leongdl wants to merge 1 commit into
OpenJobDescription:mainlinefrom
leongdl:feat/step-scope-let-run-task

Conversation

@leongdl

@leongdl leongdl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What changed

Session.run_task (the v0 Python session) gains an optional
extra_let_bindings: Optional[list[str]] parameter. When supplied, the bindings
are evaluated into the task's symbol table after Step.Name is seeded and
before session environment variables and path mapping are materialized.

This is the task-run counterpart of the parameter enter_environment already
accepts.

Why

A step's step-template-scope let bindings (RFC 0005 §3.6) had no way to reach
a task run through this API.

The bindings normally resolve at job instantiation: create_job folds
StepTemplate.let into StepTemplate.script.let via resolve_syntax_sugar.
Any caller that instantiates a job locally — openjd-cli, for example — never
sees a problem, which is why this went unnoticed.

A caller handed an un-instantiated StepTemplate sees let and script.let as
separate fields. The Deadline Cloud worker agent is such a caller: the service
serves it a resolved StepTemplate, and it does not call
resolve_syntax_sugar. For those callers the step-scope names were simply
absent from the task's symbol table, and every reference to one failed with
Undefined variable.

enter_environment gained extra_let_bindings in #333 (RFC 0008 environment
wrap actions). Task runs were not covered at the time.

Alternative considered

Callers could fold StepTemplate.let into script.let themselves by calling
resolve_syntax_sugar. Rejected: that transform is not idempotent — a second
call duplicates the bindings — so it puts a double-apply hazard in every
caller, and it duplicates model logic outside the model.

Behaviour notes

  • Script-scope let continues to shadow step-scope let, and may reference it.
    StepScriptRunner builds a child SymbolTable sourced from the session table
    and applies script.let into the child, so this falls out of the existing
    design rather than needing new ordering logic.
  • A binding that cannot be evaluated fails the action cleanly through
    _fail_action_before_start, matching enter_environment's handling. It does
    not raise out to the caller.
  • Omitting the parameter changes nothing.

Tests

Six tests in test/openjd/sessions_v0/test_session_let_bindings.py
(TestRunTaskExtraLetBindings) covering: a step-scope binding resolving in an
action; a binding referencing Step.Name (pins the seeding order); script scope
shadowing step scope; script scope referencing step scope; a failing binding
failing the action cleanly; and omitting the parameter being a no-op.

Verification performed:

  • Mutation-checked, 3/3 caught. Dropping the apply_let_bindings call,
    moving the seeding before Step.Name, and removing the try/except each
    fail at least one new test.
  • Full sessions_v0 suite: 915 passed, against a 909-passed baseline
    (+6 = the new tests). The same 6 failures occur before and after in
    test_subprocess.py::TestLoggingSubprocessSameUser::test_run_gracetime_when_process_ends_but_grandchild_uses_stdout;
    they are timing-sensitive and unrelated to this change.
  • ruff, black and mypy clean.

Related

The consuming change in the worker agent is
aws-deadline/deadline-cloud-worker-agent#1076, which is blocked on a release
containing this parameter.

`Session.run_task` had no channel for step-template-scope `let` bindings
(RFC 0005 §3.6), so a step script referencing one failed at resolve time with
`Undefined variable`. `enter_environment` has accepted `extra_let_bindings`
since OpenJobDescription#333; this adds the same parameter to `run_task`.

Why the gap was invisible: step-scope bindings resolve at job instantiation,
and `StepTemplate.resolve_syntax_sugar` folds them into the script's own `let`
so they survive into the `Job`. Any caller holding a `Job` from `create_job` —
openjd-cli, and every test in this repo — therefore never sees the problem. A
caller handed an *un-instantiated* `StepTemplate`, where `let` and `script.let`
are still separate fields, has no way to deliver them at all. That is the
Deadline Cloud worker agent, which receives one from the service; the symptom
there was 32 conformance execution cases failing with `Undefined variable` on
names their templates plainly define.

Ordering matches `enter_environment` exactly: seeded after `Step.Name`, so a
step binding may reference it, and before path mapping and env-var evaluation,
so both see a complete table. Script-scope bindings shadow step-scope ones
rather than colliding, for free — `StepScriptRunner` evaluates `script.let`
into a child table sourced from the session-scope one. Wrap-hook isolation is
unaffected: `_build_wrap_hook_scope` builds a fresh table, so step bindings
reach a wrapped `onRun` but never the hook, which is what RFC 0008 requires.

A failing binding fails the action through `_fail_action_before_start` rather
than raising out of the public API, the same contract `enter_environment`
holds.

The parameter is additive and optional, so existing callers are unaffected.

6 tests, mutation-checked 3 of 3 caught: dropping the apply, seeding before
`Step.Name`, and dropping the try/except. Coverage includes the negative
control that omitting the parameter changes nothing, and that a script-scope
binding can build on a step-scope one — the shape the failing fixtures use.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
# Script-scope bindings shadow these rather than colliding with them:
# StepScriptRunner evaluates `script.let` into a CHILD table sourced from
# this one, so a same-named script binding takes precedence.
if extra_let_bindings:

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 bindings are applied before _materialize_path_mapping, which means Session.PathMappingRulesFile and Session.HasPathMappingRules are not yet in symtab when they evaluate. So a step-scope binding that references either one fails with Undefined variable here — but the same binding works when the caller went through create_job, because instantiation folds it into script.let and StepScriptRunner evaluates that after run_task has materialized path mapping (_session.py:1327, then _runner_step_script.py:116-119). That's a divergence between the two paths the docstring above says are equivalent.

Unlike enter_environment, nothing here needs the bindings to land early: _materialize_path_mapping only writes to symtab, and _evaluate_current_session_env_vars in run_task doesn't read symtab at all (it just merges env-var dicts — there is no variables: resolution on this path). Moving the if extra_let_bindings: block to just after the _materialize_path_mapping try/except would close the gap with no other ordering consequence.

Relatedly, the justification in the comment doesn't hold for run_task: "before path mapping so {{Session.PathMappingRulesFile}} and the env-var evaluation below see a complete table" describes enter_environment's environment.variables resolution, which has no counterpart here — and as written it reads as if the bindings can see the path-mapping symbols, which is the opposite of the actual behavior.

# handed an un-instantiated StepTemplate (the Deadline Cloud worker agent,
# which receives one from the service) has `let` and `script.let` as separate
# fields, and without this parameter the step-scope names are simply absent
# from the table and every reference fails with "Undefined variable".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Coverage gap: none of the six new tests exercise extra_let_bindings with an RFC 0008 wrap environment entered, which is the branch where the new symbol placement actually matters most. On that path symtab (now carrying the step-scope bindings) becomes the base of _build_wrapped_inner_scope — so the wrapped onRun sees them — while _build_wrap_hook_scope builds a fresh table, so the hook must not. Both halves look right by inspection, but they're exactly the kind of scope-leak invariant _build_wrap_hook_scope's docstring says was previously violated and is now only guarded by that one call. A test asserting a step-scope name resolves in the wrapped action and is undefined in the hook would pin it.

@leongdl

leongdl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #357.

#356 added a source-level extra_let_bindings channel for step-scope EXPR let. The decision is that the service's resolved symbol table is the single authoritative source for those values, so #357 delivers them through the table and removes this channel again.

Closing rather than merging: the two would be redundant, and keeping both meant maintaining a documented divergence (a binding re-evaluated locally can differ from the value the service resolved) plus a double-apply hazard in the consuming agent.

#357's history contains this change and its removal; the net diff is the table work.

@leongdl leongdl closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant