feat: accept a resolved symbol table on the v0 session - #357
Conversation
`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>
The service resolves a per-step symbol table at CreateJob (Param.*, RawParam.*, Job.Name, Step.Name, step-level `let` values) and serves it as resolvedSymbolTable, but only the Rust-backed _v1 session could accept it -- the default v0 Python runtime had no channel for it, so job-template-scope `let` never reached v0 and every service-resolved symbol was re-derived or missing on the default runtime. Mirror the _v1 surface: enter_environment, exit_environment, and run_task gain resolved_symtab (a SerializedSymbolTable). Its entries seed the session symbol table first and the session's own values layer on top, matching the openjd-rs layering: Session.WorkingDirectory and the path-mapped Param.* values overwrite the base, Job.Name from the base wins over the constructor value, and script-scope `let` still shadows base symbols. A base that fails entry validation fails the action through the normal callback path instead of raising out of the public API. Extension purity holds: the only runtime import of openjd.expr is inside the conversion helper, reachable only when the caller already holds a native SerializedSymbolTable. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
exit_environment removed the environment from tracking but popped its stored Step.Name and extra `let` bindings only after the resolved symbol table deserialization and path mapping failure branches. Either failure stranded both entries permanently: identifiers are allocated per-enter, so no later exit can reach them. Move both pops up beside the wrap embedded-file record pop, so every failure branch below drains them. The replay into the symbol table stays where the table exists. The deserialization branch's comment claimed it matched the extra-lets guard, which pops before failing; it now cites the _materialize_path_mapping failure instead. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
The exit-side deserialization branch had no coverage: the existing invalid-base test exits without a base. The new test enters with step context, exits with a bad-typed base, and asserts the action fails cleanly without raising. It also asserts the stored step context was drained, which is what pins the preceding drain fix — the FAILED assertions pass either way. The drain is asserted on private dicts because the strand has no public surface; the docstring records why it still matters, namely that identifiers are caller-supplied and reusable, so a stale entry can replay on a later exit. Verified by mutation: restoring the pops below the failure branches fails this test and only this test. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
_build_wrap_hook_scope built its table with no resolved base, so an RFC 0008 hook could not resolve a name only the service-resolved base defines. openjd-rs resolves a hook against the current action's full symbol table, base included, so the same hook passes there and failed here. Thread the inner entity's base through from the three call sites (enter, exit, run_task). This does not weaken the inner-to-hook isolation the two-scope split exists for: the base a step's action carries never holds Task.Param.*, because the service copies only Param.*, RawParam.*, Job.Name, Step.Name and step-level `let` values into it. Base Step.Name does become hook-visible, which is the parity openjd-rs has. New tests cover all three hook paths, base Step.Name visibility, and an isolation control that fails if the hook scope is built by copying the inner entity's table instead. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
A wrap environment's hooks resolve in its enter-time scope, and in openjd-rs that scope is the environment's frozen resolved symbol table merged onto the action's table. Python replayed only the step-name and extra-`let` fallback, so a hook referencing its own step's context delivered through the base resolved on openjd-rs and failed here. Store each environment's converted base at enter time and seed it in _seed_wrap_env_scope. The store happens after the deserialization succeeds, not beside the step-name and extra-lets stores, so a failed deserialization cannot leave a base behind. The base seeds before the existing replay, which keeps the fallback authoritative when the two disagree; exit drains the entry with the other two. Tests: the wrap env's own base resolves in a later task's hook, does not reach the wrapped action's scope, and the fallback wins over a disagreeing base. The exit-drain test now asserts this dict too. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Trim restated wording and correct one claim: a stranded per-enter entry was described as unreachable, but identifiers may be supplied by the caller and reused after an exit, so a re-entered identifier would replay stale context. That is why the drain matters. 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}
| identifier = self._wrap_env_identifier(wrap_env) | ||
| # Values only, exactly as _symbol_table seeds a base: the EXPR types | ||
| # ride the values the engine already built. | ||
| for base_name, base_value in self._environment_resolved_bases.get(identifier, {}).items(): |
There was a problem hiding this comment.
_seed_wrap_env_scope seeds the wrap environment's remembered base last, which inverts the layering that _symbol_table establishes.
_build_wrap_hook_scope builds hook_symtab via _symbol_table(version, resolved_base=...), where the base is seeded first and then session-derived values deliberately overwrite it — Session.WorkingDirectory, and notably the path-mapped Param.* values from processed_parameter_value(). This loop then writes the wrap env's own base entries over that result.
Since the base is documented to carry Param.*/RawParam.* and to_symtab(path_format=...) only chooses a rendering format (it does not apply this session's path-mapping rules), a wrap hook that references {{Param.SomePath}} will see the unmapped service value here, not the session's remapped one — the opposite of what test_path_param_remapped_over_base pins for the non-wrap path.
There's a secondary inconsistency: this loop writes values without touching symtab.expr_types, so for a name where _symbol_table recorded a type (e.g. a Param.* PATH) the value now comes from the base while the recorded EXPR type is the session's — value and type no longer come from the same source.
Suggest restricting this to names the session-scope table did not already define, or re-applying the session-derived overrides after seeding, so the hook scope keeps the same "runtime locals win over the base" ordering as every other scope.
| # wrap_env.script. | ||
| hook_symtab = self._build_wrap_hook_scope(step_script.revision, symtab) | ||
| hook_symtab = self._build_wrap_hook_scope( | ||
| step_script.revision, symtab, resolved_base=resolved_base |
There was a problem hiding this comment.
Threading the inner entity's resolved_base into _build_wrap_hook_scope punches a hole in the isolation that method exists to enforce.
_build_wrap_hook_scope's own docstring states that a wrap environment must not read "the extra_let_bindings the inner environment was entered with", and test_inner_extra_let_bindings_are_not_in_the_hook_scope pins that. But the docstring here also acknowledges the base carries "step-level let values" — so the exact same inner step-scope values now reach the hook whenever the caller supplies them via resolved_symtab instead of extra_let_bindings.
_seed_wrap_env_scope only masks the overlap: it overwrites names the wrap env's own base/lets also define, so an inner-step binding name that the wrap env does not itself define stays visible to the hook. Same for inner Step.Name when the wrap env was entered without one (the new test_base_step_name_is_hook_visible documents that as intended, but it is the mirror of test_running_step_name_is_not_in_the_hook_scope, which asserts the opposite for the non-base channel).
Net effect: the two channels for identical data have opposite isolation properties, and which one a caller uses is not something the template author controls. If parity with openjd-rs really requires the inner base to be hook-visible, it would be worth reconciling the extra_let_bindings/Step.Name isolation rules with it rather than leaving them contradictory — otherwise consider filtering the inner base down to the session-scope names (Param.*/RawParam.*/Job.Name) before seeding the hook scope.
| # the interceptor is always a different, still-entered outer | ||
| # environment, and an environment that has exited can never intercept | ||
| # again. | ||
| self._environment_resolved_bases.pop(identifier, None) |
There was a problem hiding this comment.
The exit side has an asymmetry with the other two remembered pieces of enter-time context.
Step.Name and extra_let_bindings are stored at enter time and automatically replayed on exit (lines 1196-1200) precisely so "its onExit resolves in the same scope as its onEnter". The base is stored the same way in _environment_resolved_bases, but on exit it is only dropped here — never used. The exit action's base comes solely from the caller re-passing resolved_symtab=.
So an environment entered with resolved_symtab=base and exited without it silently resolves its onExit in a different scope than its onEnter: any base-only name in onExit (or in an onExit arg / embedded file) fails to resolve, while the identical value delivered through extra_let_bindings would have worked. The docstring at line 1104 turns this into a caller obligation, but nothing enforces it and the failure mode is a resolution error at exit — the worst time, since exit is the cleanup path.
Since the value is already tracked, using the stored base as the fallback when resolved_symtab is None would make the three channels behave consistently and remove the obligation:
resolved_base = stored_base if resolved_symtab is None else self._resolved_base_entries(resolved_symtab)
| # matching the openjd-rs runtime's layering over the same table. | ||
| # A base entry for a name the session does not know survives. | ||
| if resolved_base: | ||
| for base_name, base_value in resolved_base.items(): |
There was a problem hiding this comment.
Path mapping is only re-applied to base Param.* entries that also appear in self._job_parameter_values.
The base seeded here is produced by to_symtab(path_format=host_format), which only chooses a rendering format — it does not apply this session's _path_mapping_rules. Mapping is re-applied further down solely by iterating self._job_parameter_values and calling processed_parameter_value(). So for any Param.X / RawParam.X the base carries but the session's job_parameter_values does not, the base's value survives untouched and reaches the action unmapped.
test_path_param_remapped_over_base covers only the overlapping case (the parameter is in both), so the divergent case is untested. Since path mapping is the mechanism that keeps a job from being handed host paths it should not see, a base-supplied PATH that bypasses it is more than a cosmetic mismatch.
Two options depending on the intended contract: either run base entries under the Param./RawParam. prefixes through apply_mapping when the session has rules, or document that the caller must pass a job_parameter_values that is a superset of the base's Param.* names (and ideally assert it) so the gap cannot open silently.
The resolved symbol table is now the single authoritative channel for step-scope EXPR `let` values. `extra_let_bindings` was the source-string fallback that predated it; carrying both meant two channels for the same values, with a documented divergence where the locally re-evaluated binding won over the value the service resolved. Removed from `run_task`, `enter_environment` and `exit_environment`, along with the `_environment_extra_let_bindings` tracking dict and the replay in `_seed_wrap_env_scope`. That replay was the method's only failure path, so its return type is now `None` and the three call sites no longer branch on it. `apply_let_bindings` stays imported: it is still used by `_build_wrapped_inner_scope` for inner script lets. The environment case is safe because the service stores a step-scoped environment with the owning step's resolved symbol table, so an ENV_ENTER already receives `Step.Name` and the step's template-scope `let` values in its table. Nothing needed the source-string channel. `step_name` is kept on both methods. On `run_task` it feeds RFC 0008's `WrappedStep.Name`, an injected overlay symbol the table does not carry. On `enter_environment` it is redundant with the base but retained for a caller that has a step name and no resolved table. Tests whose only subject was the parameter are deleted; those that used it to set up step context now deliver the same context through `resolved_symtab`, so the wrap-scope isolation and hook-seeding properties stay pinned. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
CI runs `mypy src test` (hatch.toml) and rejected the monkeypatched _build_wrap_hook_scope stand-in: a **kwargs signature is not assignable to the bound method's type once fix 1a added resolved_base, and the existing `type: ignore[method-assign]` does not cover the `assignment` error code. All 18 Python matrix legs failed at Run Linting. Give the stand-in the real signature and forward resolved_base rather than widening the suppression, so the harness observes the builder transparently instead of altering what it is meant to watch. My miss: I ran `mypy src` locally, not the repo's `mypy src test`. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| identifier: Optional[EnvironmentIdentifier] = None, | ||
| os_env_vars: Optional[dict[str, str]] = None, | ||
| extra_let_bindings: Optional[list[str]] = None, | ||
| step_name: Optional[str] = None, |
There was a problem hiding this comment.
extra_let_bindings is removed from enter_environment, but it is a released public keyword argument, and nothing in this PR marks the removal as breaking.
It shipped in 0.10.11 (df96902, called out by name in that CHANGELOG entry: "enter_environment(extra_let_bindings=...) so a steps environments see the step-level lets"), so any caller on 0.11.x that passes it gets a TypeError on upgrade. The run_task counterpart is safe to drop — 82c3be7 added it inside this same PR — but the enter_environment one is not.
The version bump makes it worse: pyproject.toml puts refactor in [tool.semantic_release] patch_tags with minor_tags = [], and the commit is refactor: Drop extra_let_bindings parameter with no BREAKING CHANGE: footer. So this lands as a patch release with no ⚠ BREAKING CHANGES note — the one signal a downstream caller has.
Two ways out, depending on how much the "single authoritative channel" goal is worth:
- Keep the kwarg accepted-and-ignored (or accepted-and-applied) for one release with a
DeprecationWarning, then drop it. Preserves the patch bump honestly. - Keep the removal, but add a
BREAKING CHANGE:footer to the commit so semantic-release bumps and documents it.
Worth confirming against the worker agent too: the commit message argues the removal is safe because "the service stores a step-scoped environment with the owning steps resolved symbol table", which is a claim about one caller, not about every caller of a public API.
| # injection to keep all three hook paths reading the same way. | ||
| if not self._seed_wrap_env_scope(hook_symtab, wrap_env): | ||
| return | ||
| self._seed_wrap_env_scope(hook_symtab, wrap_env) |
There was a problem hiding this comment.
_seed_wrap_env_scope now runs after _try_inject_wrapped_symbols, and it writes arbitrary service-supplied names — so a base entry can silently overwrite the WrappedAction.* overlay.
The comment above says ordering "is no longer load-bearing here", which is true for the direction it argues (the base cannot leak into the wrapped action's scope, since hook_symtab is a separate table). But it is load-bearing in the other direction: injection writes WrappedAction.Command, WrappedAction.Args, WrappedAction.Environment, WrappedAction.Timeout, WrappedAction.Cancelation.* and WrappedStep.Name / WrappedEnv.Name into hook_symtab, and this loop then does an unconditional symtab[base_name] = base_value for every name in the stored base.
A base carrying an entry literally named WrappedAction.Command replaces the command the hook is supposed to run on behalf of the wrapped action. Unlike the old let-binding replay, these names are not template-authored — they come off the wire in SerializedSymbolTable, and _resolved_base_entries copies every engine_tab.symbols name through unfiltered. So the one symbol group RFC 0008 says the hook must be able to trust is the one a base can redefine. The same applies to the three hook paths, since all three call this after injection.
Note this is specific to _seed_wrap_env_scope: the inner entity's base goes in via _build_wrap_hook_scope, which runs before injection, so injection correctly wins there. It's only the wrap env's own stored base that lands late.
Cheapest fix is to move the call before _try_inject_wrapped_symbols in all three paths, so the overlay stays authoritative (the reason it currently sits after — "to keep all three hook paths reading the same way" — is satisfied either way). Alternatively skip WrappedAction.* / Wrapped*.Name names when seeding.
|
This removes extra_let_bindings from enter_environment, which shipped in released 0.11.0 — so this is a backwards-incompatible public API change, not a patch. The repo's [tool.semantic_release] config has minor_tags = [] and feat in patch_tags, so a bare feat: commit will cut 0.11.1 (patch). Per the README's versioning policy ("MINOR is incremented when backwards incompatible changes are introduced to the public API"), this should cut 0.12.0. Can you add a BREAKING CHANGE: footer to the squash/merge commit so semantic-release bumps MINOR? Otherwise the removal ships silently as a patch and anyone pinned ~=0.11.0 picks it up. Note the PR description says the parameter "was added in the superseded #356" and never shipped — but it's present in mainline _session.py at 0.11.0. |
0.11.6 is the first openjd-model release that accepts an environment defining only `onExit` (openjd-model 5b9c661, released in f7ca58f). This package enters and exits environments handed to it by a caller, so a template the model previously rejected at validation is one it can now be given. 0.11.5 carried only an openjd-rs crate dependency bump. The upper bound is unchanged: 0.11.6 is the newest release and `< 0.12` still holds. BREAKING CHANGE: `extra_let_bindings` was removed from `Session.enter_environment`, `Session.exit_environment` and `Session.run_task` in #357 (457a364). It was public in 0.11.0, so the removal is backwards incompatible for the public API. That squash commit landed without this footer, so semantic-release would otherwise cut the removal as a patch and anyone pinned `~=0.11.0` would pick it up. Callers should deliver step-scope EXPR `let` values through `resolved_symtab` instead. Per the README's versioning policy this bumps MINOR (0.12.0); `major_on_zero = false` keeps it inside 0.x. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
3 participants
What changed
The v0
Sessiongainsresolved_symtab: Optional[SerializedSymbolTable]onenter_environment,exit_environmentandrun_task, matching the_v1(Rust-backed) session's parameter of the same name. Its entries seed the
session symbol table first, and the session's own values layer on top.
Why
create_jobresolves a symbol table per step and environment, and the_v1session already accepts it. The v0 session had no way to take one, so acaller holding a pre-resolved table could only throw it away and re-derive what
it could. Two consequences: job-template-scope
lethas no channel into v0 atall, and every symbol the producer already resolved is recomputed, which is the
divergence risk a pre-resolved table exists to remove.
Layering
The contract is openjd-rs
Session::build_symbol_table: the base is a base, andruntime-local values overwrite it.
to_symtab(host format)_resolved_base_entriesParam.*PATH / LIST[PATH] from the session's own valuesParam.*/RawParam.*loop overwritesSession.WorkingDirectoryTask.*Job.Namerides the basejob_nameseeds only when the base lacks itHalf of a v0 task table cannot come from a producer at all —
Session.WorkingDirectory,Task.File.*/Env.File.*, anything path-mapped areallocated per session on the host — which is why the base is layered under them
rather than trusted wholesale.
RFC 0008 wrap hooks resolve against the base too, on both channels openjd-rs
uses: the inner entity's base (its action's table) and the wrap environment's
own frozen enter-time base.
Task.Param.*stays out of hook scope, as before;the service never copies task parameters into a base, so seeding it cannot leak
them.
Behaviour notes
_fail_action_before_start, the same shapeextra_let_bindingsfailures use.It does not raise out of the public API.
ExprValues and keep their types, so arithmetic on abase integer works.
evaluate_let_bindingsalready storesExprValues inthese tables, so this is not a new kind of value.
Extension purity
SerializedSymbolTableis imported underTYPE_CHECKINGonly, and the oneruntime import (
PathFormat) sits inside the conversion helper, reachable onlywhen a caller passes a table and therefore already holds a native object.
test/openjd/test_import_purity.pystays green.Tests
test_session_resolved_symtab.pycovers a base-only symbol resolving in anaction, type fidelity,
Job.Nameprecedence, runtime locals overriding thebase, PATH re-mapping over the base, both environment entry points, an invalid
base failing cleanly on
run_taskand on both environment paths, script-scopeletshadowing the base, and omitting the parameter.test_wrap_scope_isolation.pygains nine covering hook scope on all three hookpaths, the wrap environment's own base, and negative controls that
Task.Param.*and the wrap environment's base still do not reach the wrappedaction.
Removing
extra_let_bindingsdeleted eight tests whose only subject was thatparameter. Tests that used it to set up step context were rewritten to deliver
the same context through a resolved table, so the properties they pinned are
still pinned — including the wrap-scope isolation cases, both of which were
mutation-checked after rewriting.
Verification:
named test failed each time, including two mutants that initially survived:
seeding the base after the step-context replay instead of before, and building
hook scope by copying the inner table instead of seeding the base. Tests were
added for both.
sessions_v0suite: 928 passed, against 926 before this branch. Thesame six 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.
ruff check,black --checkandmypyclean.worker change: 31 of 32
EXPR/jobsconformance cases pass, including all fivestep-scope
letcases. The one failure is an unrelated worker-side defect witha served FEATURE_BUNDLE_1 simple action, tracked separately.
Not verified: Windows. The host path-format branch (
os.name == "nt") and theWindows arm of the path-mapping test were not exercised; this was developed on
macOS.
Related
Consumed by aws-deadline/deadline-cloud-worker-agent#1077. Supersedes #356.