feat: deliver step-scope let to Python session - #1076
Conversation
A step's step-template-scope `let` bindings (RFC 0005 section 3.6) never reached the task running under the Python session runtime, so any reference to a step-scope name failed to resolve. Step-scope bindings are normally resolved at job instantiation: create_job folds StepTemplate.let into StepTemplate.script.let via resolve_syntax_sugar. Any caller that instantiates a job locally therefore never sees a problem. This agent is handed a service-resolved but un-instantiated StepTemplate, where `let` and `script.let` are separate fields, and it never calls resolve_syntax_sugar -- so the step-scope names were simply absent from the task's symbol table. Environments already had this channel: enter_environment takes extra_let_bindings, and the scheduler extracts step_template.let for ENV_ENTER actions. Task runs were left out. RunStepTaskAction now passes step_template.let through Session.run_task to the runtime. The Python runtime forwards it to the openjd session's new run_task(extra_let_bindings=...) parameter. The Rust runtime accepts and ignores it: those same values already arrive inside resolved_symbol_table_json, which the _v1 session uses as the base of its per-action symbol table, so forwarding would define them twice. Folding StepTemplate.let into script.let inside this package was considered and rejected: resolve_syntax_sugar is not idempotent, so a second call duplicates bindings, and it would duplicate model logic. Raises the openjd-sessions floor to 0.11.1, the release that adds the run_task parameter. Against 0.11.0 the new call raises TypeError. Tests: 6 end-to-end tests drive the real chain (StepDetails.from_boto -> RunStepTaskAction -> PythonSessionRuntime -> openjd session -> real subprocess) and assert on the text the task actually emitted, including negative controls for a step with no `let` and for script-scope `let` alone. All 5 mutants were caught, including reverting the action to its pre-fix state; the controls survive each one. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
github-actions
Bot
added
the
waiting-on-maintainers
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| class TestRunTaskStepScopeLetBindings: |
There was a problem hiding this comment.
The new class is inserted in the middle of TestRunAttachmentSyncTask, so test_propagates_exception_from_openjd_session (previously the second test of TestRunAttachmentSyncTask, now at line 2608) is silently re-parented into TestRunTaskStepScopeLetBindings.
That test exercises _run_attachment_sync_task / _run_task_without_session_env and has nothing to do with step-scope let, so it now lives under a class whose docstring contradicts it. It still passes, but the grouping is wrong and it will confuse anyone reading either class.
Moving the new class to after test_propagates_exception_from_openjd_session (i.e. append it at the end of the TestRunAttachmentSyncTask block rather than splicing into it) keeps both classes coherent.
| # values it carries are already inside resolved_symbol_table_json, which | ||
| # create_job pre-resolved and which the _v1 session takes as the base of | ||
| # its per-action table. Applying them a second time here would duplicate | ||
| # the definitions. Mirrors how enter_environment drops it. |
There was a problem hiding this comment.
The stated justification for dropping extra_let_bindings here — "already inside resolved_symbol_table_json" — only holds when that JSON is actually present and parses. Two paths where it is not:
resolvedSymbolTableis an optional field on the entity (Field(key="resolvedSymbolTable", ..., required=False)instep_details.py, read with.get(..., None)), soresolved_symbol_table_jsoncan legitimately beNone._parse_resolved_symtab(rust.py:272-284) deliberately degrades toNoneon malformed JSON, logging only a warning.
In either case resolved_symtab=None is passed and extra_let_bindings is discarded, so the Rust runtime silently loses step-scope let — the exact defect this PR fixes, reproduced on the other runtime. And because the drop is unconditional, the failure mode is an Undefined variable at task run with no indication that bindings were available but thrown away.
Consider forwarding extra_let_bindings when resolved_symtab is None (fallback rather than unconditional drop), or at minimum logger.warning when extra_let_bindings is non-empty and resolved_symtab is None, so the loss is visible.
| """ | ||
| template: dict[str, Any] = { | ||
| "name": "MyStep", | ||
| "script": {"actions": {"onRun": {"command": "echo", "args": args}}}, |
There was a problem hiding this comment.
echo is not a real executable on Windows — it is a cmd.exe builtin, and there is no echo.exe on PATH. openjd.sessions launches onRun commands directly via subprocess without a shell, so on Windows every test in this file that asserts ActionState.SUCCESS will fail (the process cannot be spawned at all).
code_quality.yml runs the unit suite on windows-latest across Python 3.9–3.13, so this is not hypothetical. Note the only other "command": "echo" in the unit suite (test_session_events.py:525) is inert log-redaction fixture data that is never executed, so there is no precedent here.
Existing tests that need a real subprocess use sys.executable (see test_run_attachment_upload.py:41). Using sys.executable with -c to print the resolved value would make this portable, e.g.
"script": {"actions": {"onRun": {"command": sys.executable, "args": ["-c", "import sys; print(sys.argv[1])", *args]}}}Alternatively add a pytest.mark.skipif(platform.system() == "Windows", ...) at class level, though that gives up the coverage on a platform the agent supports.
| step_let=["shared = 'from step'", "base = 'step base'"], | ||
| script_let=["shared = 'from script'", "derived = base + ' + script'"], | ||
| args=["SHARED:{{ shared }} DERIVED:{{ derived }}"], | ||
| ) |
There was a problem hiding this comment.
These assertions depend on openjd.sessions.LOG records reaching caplog, which only works if that logger propagates to the root logger. This repo's own code says it does not — see sessions/session.py lines 237-240, which state the OpenJD logger "is already set up to not propagate to the agent log".
caplog captures via a handler installed on the root logger, and caplog.set_level(logging.INFO) adjusts levels but does not touch propagate. If openjd.sessions.LOG.propagate is False, caplog.messages is empty and every assert any(... in m for m in caplog.messages) in this file fails — meaning these tests would silently not be verifying the fix.
Consistent with this: no other unit test in test/unit/sessions/ asserts on caplog for openjd output; test_session.py line 1038 patches OPENJD_LOG directly instead.
Worth confirming by running the file locally. If propagation is off, attach a handler to openjd.sessions.LOG directly (or enable propagation for the test duration) rather than relying on the root handler.
|
Superseded by #1077. #1076 passed step-scope EXPR Closing rather than merging, since the two would be redundant. #1077's history contains this change and its removal; the net diff is the table work plus two fixes found while testing it on a fleet. |
1 participant
Important
Blocked on a dependency release. This needs
OpenJobDescription/openjd-sessions-for-python#356 merged and released as
openjd-sessions0.11.1. The pin in this PR points at 0.11.1, so CI cannotinstall until that release exists. Please do not merge before then.
What changed
RunStepTaskActionnow passes the step's step-template-scopeletbindings(
StepTemplate.let) through to the session runtime, and the Python runtimeforwards them to
openjd.sessions.Session.run_task(extra_let_bindings=...).Plumbing, five files:
sessions/actions/run_step_task.pyextra_let_bindings=self._details.step_template.letsessions/session.pysessions/runtime/_abc.pyrun_tasksessions/runtime/python.pysessions/runtime/rust.pyWhy
A step's step-template-scope
letbindings (RFC 0005 §3.6) never reached thetask when running under the Python session runtime, so any reference to a
step-scope name failed to resolve with
Undefined variable.Those bindings normally resolve at job instantiation —
create_jobfoldsStepTemplate.letintoStepTemplate.script.letviaresolve_syntax_sugar.This agent is handed a service-resolved but un-instantiated
StepTemplate,where
letandscript.letarrive as separate fields, and it never callsresolve_syntax_sugar. So the fold never happened for what this agent sees, andthe step-scope names were absent from the task's symbol table.
Environments already had this channel:
enter_environmenttakesextra_let_bindings, andscheduler/session_queue.pyextractsstep_template.letforENV_ENTERactions. Task runs were left out.Why the Rust runtime ignores it
The
_v1session receives the same values insideresolved_symbol_table_json,which
create_jobpre-resolved and which that session uses as the base of itsper-action symbol table. Forwarding the bindings as well would define the same
names twice. This matches how
enter_environmentalready drops the parameter onthat path. The behaviour is pinned by a test, so a future change to forward them
trips it rather than passing silently.
Alternative considered
Fold
StepTemplate.letintoscript.letin this package by callingresolve_syntax_sugar. Rejected: that transform is not idempotent — a secondcall duplicates the bindings — so it puts a double-apply hazard in this code
path, and it duplicates model logic outside the model.
Tests
13 new tests.
test/unit/sessions/test_step_scope_let_end_to_end.py(6) drives the real chainand asserts on the text the task actually emitted, not on mock call arguments:
Only the scheduler-facing
Sessionorchestrator is stood in for; itsrun_taskis a one-line pass-through, pinned separately in
test_session.py. The entity isbuilt from the real served
BatchGetJobEntitypayload shape, so the testexercises the actual condition —
letandscript.letas separate unfoldedfields.
Covered: a step-scope binding resolving in the command; both scopes resolving
with script scope shadowing step scope; a binding referencing
Step.Name; anunresolvable binding failing the action cleanly; and two negative controls — a
step with no
let, and script-scopeletalone — so an over-correction failssomething.
The remaining 7 pin the plumbing in
test_python.py,test_rust.py,test_session.pyandactions/test_run_step_task.py.Verification performed:
making it send
None, dropping the Python adapter's forwarding, dropping theenter_environmentforwarding, and making the Rust adapter forward — eachfails at least one new test, and the negative controls survive every one.
3149-passed / 47-skipped baseline. The +13 is exactly the tests added; no
pre-existing test changed behaviour.
ruff format,ruff checkandmypyclean across 217 files.