feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005-0008)#318
feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005-0008)#318leongdl wants to merge 2 commits into
Conversation
Implement the RFC 0008 WRAP_ACTIONS runtime in the pure-Python (v0) session: dispatch onWrapEnvEnter/onWrapTaskRun/onWrapEnvExit in enter_environment/run_task/exit_environment, seed WrappedAction.*/WrappedEnv.Name/WrappedStep.Name and resolve them through the model's Rust EXPR bindings, and add run_wrap_action to both script runners (plus an optional step_name kwarg on run_task). Includes end-to-end tests for all three hooks and the no-wrap path. Pin openjd-model >= 0.11,< 0.12 -- the model release that ships the EXPR (RFC 0005-0007) + WRAP_ACTIONS (RFC 0008) bindings (build_symbol_table, job_parameter_type_expr_spec) and the wrap-hook model. The Rust-backed openjd.sessions._v1 implementation (and its test tree) is intentionally excluded from this PR and tracked separately. 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}
| session_files_directory=self.files_directory, | ||
| ) | ||
| self._runner.exit() | ||
| if wrap_action is not None: |
There was a problem hiding this comment.
When an onExit is replaced by onWrapEnvExit, the 5-minute default exit timeout is lost. The normal exit path calls self._runner.exit(), which runs _run_env_action(onExit, default_timeout=_ENV_EXIT_DEFAULT_TIMEOUT) (5 minutes). Here run_wrap_action(wrap_action) is called with no default_timeout, so unless the wrap action itself declares a timeout, the wrapped exit action runs indefinitely — a safety regression vs. the unwrapped exit. Consider passing default_timeout=_ENV_EXIT_DEFAULT_TIMEOUT on the exit dispatch.
| if self.state != ScriptRunnerState.READY: | ||
| raise RuntimeError("This cannot be used to run a second subprocess.") | ||
| log_subsection_banner(self._logger, "Phase: Setup") | ||
| self._run_action(action, self._symtab, default_timeout=default_timeout) |
There was a problem hiding this comment.
Cancelation of a running wrap task action uses the wrong action's cancelation config. StepScriptRunner.cancel() reads self._script.actions.onRun.cancelation (mode + notifyPeriodInSeconds). When run_wrap_action is the action actually running, cancelation should follow the wrap action's cancelation, but here it follows the inner step's onRun. (Contrast with EnvironmentScriptRunner.run_wrap_action, which sets self._action = action so its cancel() uses the wrap action.) Consider recording the running action and having cancel() derive its method from it.
| else: | ||
| args.append(str(value)) | ||
|
|
||
| wrapped_env = [f"{name}={value}" for name, value in self._openjd_defined_env().items()] |
There was a problem hiding this comment.
WrappedAction.Environment is asymmetric between enter and exit. _openjd_defined_env() iterates self._environments_entered. In enter_environment, seeding happens after the inner env is appended and its variables recorded in _created_env_vars, so the wrapped env's own OpenJD-defined variables are included. In exit_environment, seeding happens after the env has been popped from _environments_entered and deleted from _environments, so the env being exited contributes nothing to WrappedAction.Environment — even though its onExit would normally run with those variables set. If RFC 0008 intends WrappedAction.Environment to reflect the wrapped action's own environment, the exit case is missing the wrapped env's variables. Worth confirming against openjd-rs.
| does not yet include the wrapped environment's embedded files | ||
| (``Env.File.*``) — those are materialized inside the runner. | ||
| """ | ||
| command = inner_action.command.resolve(symtab=symtab) |
There was a problem hiding this comment.
inner_action.command.resolve(...) / arg.resolve(...) here are unguarded. The normal action path (_run_action) wraps the same .resolve() calls in try/except FormatStringError and, on failure, sets the runner to FAILED and invokes the callback so the Session reports an action failure cleanly. Per the docstring's own caveat, the inner action's command/args may reference Env.File.* symbols that are not yet in symtab (embedded files aren't materialized until inside the runner). If the wrapped action does reference such a symbol, .resolve() raises FormatStringError, which propagates out of enter_environment / run_task as an unhandled exception with _action_state left at RUNNING, rather than being surfaced as a FAILED action. Consider guarding the resolution and routing failures through the normal action-failed path.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| expressions = arg.expressions | ||
| if not expressions or expressions[0].expression is None: | ||
| continue | ||
| symtab[name] = expressions[0].expression.evaluate( |
There was a problem hiding this comment.
_apply_let_bindings evaluates each binding RHS with expressions[0].expression.evaluate(...) but does not guard against failure. This method is called from enter_environment, exit_environment, and run_task before _action_state is set to RUNNING, and none of those call sites wrap it in a try/except.
As the docstring itself notes, a binding RHS that references a not-yet-materialized symbol (e.g. Env.File.* / Task.File.*) — or any undefined symbol / type error at runtime — will raise FormatStringError (or similar) here. That propagates out of enter_environment / run_task / exit_environment as an unhandled exception, rather than being surfaced as a FAILED action through the normal _action_callback path the way action-resolution failures are. The Session is left without a clean failure signal.
Consider routing evaluation failures through the same action-failed path used for action resolution (set the runner/state to FAILED and invoke the callback), consistent with the unguarded-resolution concern already raised on the wrap-action seeding.
| expressions = arg.expressions | ||
| if not expressions or expressions[0].expression is None: | ||
| continue | ||
| symtab[name] = expressions[0].expression.evaluate( |
There was a problem hiding this comment.
_apply_let_bindings stores each binding's native typed result directly into symtab[name] but, unlike _seed_wrapped_action_symbols, never records the value's type in symtab.expr_types. The wrap-seeding code does this deliberately, with the comment "Record EXPR types so the (Rust) build_symbol_table coercion produces the right typed values for these symbols."
If that coercion defaults symbols without an expr_types entry to STRING, then a let binding whose value is not a string — e.g. LIST[STRING], INT, BOOL — would be coerced incorrectly when an action's {{ }} expression references it. The existing tests only cover an INT rendered straight into a string (doubled = 21 * 2 → "42"), which survives string coercion; a list-valued binding referenced in args would not be covered and is the likely failure case.
Consider recording the evaluated value's EXPR type in symtab.expr_types here as well, mirroring the wrap-action path.
| @@ -142,6 +142,31 @@ def _run_env_action( | |||
| self._action = action | |||
There was a problem hiding this comment.
Review — PR #318: feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005–0008)
Repo: OpenJobDescription/openjd-sessions-for-python · wrap-actions-v0 → mainline
Author: leongdl · +596 / −5, 6 files · state: open
Reviewed against: the Rust reference at openjd-rs/crates/openjd-sessions/src/session.rs
and the openjd-model Python/Rust bindings.
Summary
This PR teaches the pure-Python (v0) Session two new tricks from the OpenJD
expression RFCs. First, RFC 0007 let bindings: enter_environment,
exit_environment, and run_task now evaluate a script's let list into the
symbol table before resolving the action, so an action's {{ }} expressions can
reference computed values. Second, RFC 0008 WRAP_ACTIONS: when an active
"wrap" environment declares onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit,
that hook runs in place of an inner environment's onEnter, a step's onRun,
or an inner environment's onExit — with WrappedAction.* / WrappedEnv.Name /
WrappedStep.Name seeded from the inner action and resolved through the model's
Rust EXPR engine. A new run_wrap_action entry point is added to both script
runners, run_task gains an optional step_name, and the openjd-model pin
moves to >= 0.11,< 0.12.
The change is well-structured and well-commented, the dispatch logic faithfully
mirrors the Rust reference (innermost-wins wrap selection, never-self-wrap,
host-vars-excluded environment), and the two new test files cover all three
hooks plus the no-wrap and chained-let paths end-to-end. The findings below are
mostly about the Python↔Rust boundary and one real edge-case bug.
Call stacks (Phase 1 — required)
Full traces with file:line hops and boundary crossings are saved to:
review/call-stacks/318-wrap-actions-v0.md
The three primary flows:
Flow 1 — run_task with onWrapTaskRun:
run_task → _apply_let_bindings → _active_wrap_env → _seed_wrapped_action_symbols
──BOUNDARY──▶ inner_action.command/args.resolve (Rust EXPR) ◀── str
→ StepScriptRunner.run_wrap_action → _run_action (skips embedded files)
Flow 2 — enter_environment with onWrapEnvEnter:
enter_environment → _apply_let_bindings → _wrap_env_excluding(self) → seed
→ EnvironmentScriptRunner.run_wrap_action (sets self._action; step runner does not)
Flow 3 — _seed_wrapped_action_symbols records symtab.expr_types; _apply_let_bindings does not.
Key insight: wrap actions resolve their {{ WrappedAction.* }} references
through the same Rust build_symbol_table coercion the rest of the model uses —
which is why _seed_wrapped_action_symbols registers expr_types for each seeded
symbol. The dispatch is correctly gated: nothing changes for templates that don't
declare a let field or a wrap hook.
Findings
Correctness
1. _session.py:1577 — int(inner_action.timeout) raises on a FormatString timeout (FEATURE_BUNDLE_1). (PLAUSIBLE)
timeout = int(inner_action.timeout) if inner_action.timeout else 0Action.timeout is Optional[Union[PositiveInt, FormatString]]
(openjd-model .../v2023_09/_model.py:446). Under the FEATURE_BUNDLE_1 extension
a timeout may be a {{ }} format string (e.g. one referencing a task parameter)
that is still unresolved at runtime. int("{{ Task.Param.t }}") raises
ValueError: invalid literal for int() — which here would crash wrap-action
seeding rather than produce WrappedAction.Timeout.
- Why it matters: the Rust reference does not do this — it calls
resolve_action_timeout(...)(runner/mod.rs:234), which resolves the format
string through EXPR then parses the integer. This is a genuine Python-vs-Rust
divergence (the ★ parity rule), latent until a wrapped action carries a
FormatString timeout. - Suggested fix: resolve the timeout the same way the command/args are resolved
a few lines up, then coerce: e.g. resolveinner_action.timeoutvia the
FormatString path when it's aFormatString, fall back toint(...)only for
the already-numeric case. Mirrorresolve_action_timeout's "0/empty = unset"
semantics. - Conformance: per the parity rule this is spec-observable (it changes whether a
valid wrapped-action template runs), so it warrants a conformance fixture once
fixed — both polarities (FormatString timeout resolves vs. malformed rejected).
Boundary / exceptions / reuse
2. _session.py:1184 _apply_let_bindings reimplements let evaluation in Python instead of calling the Rust binding the model already exposes. (reuse — item #5)
The model already ships openjd._openjd_rs.evaluate_let_bindings(bindings, symtab, *, profile=None)
(declared in _openjd_rs.pyi:3595, implemented at
rust-bindings/src/model/create_job_fns.rs:218), and the Rust session uses
exactly that (let_bindings.rs → openjd_model::evaluate_let_bindings). This
PR instead re-parses each RHS as ArgString_2023_09("{{ " + rhs + " }}") and
calls .evaluate(...) per binding in Python.
- Why it matters: a parallel Python implementation of
letsemantics will
drift from the canonical Rust one (binding scope/ordering, error messages, the
name = exprsplit, profile/function-library handling). The Rust path takes the
wholebindingslist and the function library in one call; the Python loop does
a naivepartition("=")that, e.g., would mishandle an=inside the RHS
expression differently than the model'sparse_let_bindingsvalidator. It's
also ~40 lines that could be ~3. - Suggested fix: call
evaluate_let_bindings(let_bindings, symtab, profile=...)
and merge the returned symbol table, matching the Rust session. This also gets
theexpr_typeshandling (Finding 3) for free, since the binding goes through
the same builder.
3. _session.py:1184 — _apply_let_bindings stores values without recording symtab.expr_types, unlike _seed_wrapped_action_symbols. (PLAUSIBLE)
_seed_wrapped_action_symbols carefully registers expr_types for each seeded
symbol so the Rust build_symbol_table coerces it correctly on the next resolve;
_apply_let_bindings does symtab[name] = expression.evaluate(...) with no
expr_types entry.
- Why it matters: if a
letbinds a non-scalar (a list) and a later
action/arg references it expecting list-flattening, the absence of a type tag
means coercion falls back to inference — the exact silent-revert hazard the
model'sexpr_typesfield exists to prevent. The chained-int test passes, but
there's no test for alet-bound list consumed by an arg. - Suggested fix: adopting Finding 2 (call the Rust binding) likely resolves this
structurally. If keeping the Python path, record the result's EXPR type in
symtab.expr_types[name]. Either way, add a regression test for alet-bound
list referenced fromargs.
4. _runner_env_script.py:166 vs _runner_step_script.py — run_wrap_action cancel asymmetry. (PLAUSIBLE / worth a comment)
The env runner's run_wrap_action sets self._action = action (line 166); the
step runner's does not. On cancel, the env runner reads self._action.cancelation
(the wrap action's config) while the step runner reads
self._script.actions.onRun.cancelation (the inner action's config).
- Why it matters: the process actually running is the wrap action, so the env
runner's choice is arguably the correct one — which makes the step runner the
odd one out (it would apply the inner onRun's cancelation to a wrap process).
Either way the two runners disagree, and nothing documents why. - Suggested fix: decide which action's cancelation governs a wrapped run
(likely the wrap action's, since that's the live process), apply it
consistently in both runners, and add a one-line comment. Confirm the intended
behavior against the Rust runner'srun_wrap_action.
Cleanup / minor
5. _session.py:1572 & :1585 — defensive getattr(symtab, "expr_types", ...) / # type: ignore[attr-defined] are unnecessary. (dead/defensive code)
SymbolTable.expr_types is a real declared field initialized in __init__ and
copied in union (openjd-model .../_symbol_table.py:22,32,76). The
getattr(symtab, "expr_types", None) or {} fallback and the
# type: ignore[attr-defined] on the assignment treat it as optional/untyped.
- Suggested fix: use
symtab.expr_typesdirectly (it's always a dict) and drop
thetype: ignore. Minor, but it removes a "this attribute might not exist"
signal that's no longer true.
6. _session.py enter_environment/exit_environment — getattr(env_script, "let", None). (minor)
EnvironmentScript declares let: Optional[list[str]] as a real field, so the
getattr(..., "let", None) guard (vs step_script.let used directly in
run_task) is inconsistent and slightly obscures intent. If the guard is only
for the env_script is None case, the surrounding if env_script is not None
already covers it. Prefer direct attribute access for symmetry with run_task.
What's good
- Faithful parity with the Rust reference on the hard parts:
innermost-wins_active_wrap_env,_wrap_env_excludingfor never-self-wrap,
and_openjd_defined_env()excluding host-inherited variables — all match
session.rsline-for-line in intent. The docstrings even cite the Rust
behavior they mirror. - Honest first-cut limitations. The "does not materialize embedded files /
evaluate let" caveat onrun_wrap_actionis documented in both runners and
matches the Rustrun_wrap_action, rather than being a silent gap. - Good test coverage for new behavior: all three wrap hooks end-to-end, the
no-wrap negative path, the "inner actions never ran" assertions (proving
replacement, not augmentation), and chainedletordering. - The
expr_typesdiscipline in_seed_wrapped_action_symbolsshows correct
understanding of the model's coercion contract.
Refuted / considered but not flagged
_active_wrap_envafter pop inexit_environment— the env is popped from
_environments_enteredbefore_active_wrap_env()is called, so it correctly
returns the outer wrap env. Matches the Rust comment atsession.rs:1125. ✅wrapped_step_name=step_name if step_name is not None else ""— defaulting
to""rather than raising is intentional (the kwarg is optional). Harmless.- List flattening in arg resolution (
str(element)per element) — broadly
matches the Rustto_display_string()per element; not flagged, but worth a
spot-check that scalar coercion (e.g. bool/decimal in a list) renders
identically across the two (alet/wrap list of bools would be the test).
Suggested next steps (only if you want them)
- Fix Finding 1 (timeout) and Finding 2 (reuse the Rust
evaluate_let_bindings)
— these are the two with real behavioral weight. - Add the two regression tests called out (FormatString timeout in a wrapped
action;let-bound list consumed by an arg). - Add a conformance fixture for the timeout-resolution behavior in
openjd-specificationsonce Finding 1 is fixed (parity rule).
| @@ -142,6 +142,31 @@ def _run_env_action( | |||
| self._action = action | |||
There was a problem hiding this comment.
PR #318 — WRAP_ACTIONS runtime + EXPR let integration: key call stacks
Repo: OpenJobDescription/openjd-sessions-for-python · branch wrap-actions-v0
What the PR does: implements the RFC 0008 WRAP_ACTIONS runtime and RFC 0007
let-binding evaluation in the pure-Python (v0) Session. Wrap hooks
(onWrapEnvEnter / onWrapTaskRun / onWrapEnvExit) declared on an active
"wrap" environment run in place of an inner environment's onEnter / a step's
onRun / an inner environment's onExit, with WrappedAction.* /
WrappedEnv.Name / WrappedStep.Name seeded into the symbol table and resolved
through the model's Rust-backed EXPR engine.
TL;DR
The v0 session previously ran each environment/step action directly. This PR
inserts two new behaviors ahead of every action dispatch:
letbindings (RFC 0007) —enter_environment/exit_environment/
run_tasknow call a newSession._apply_let_bindings(...)which parses each
"name = expr"RHS as a standalone EXPR{{ }}expression, evaluates it
against the symbol table built so far (so later bindings see earlier ones), and
stores the native typed result under the bound name.- WRAP_ACTIONS (RFC 0008) — before each of the three dispatch sites, the
session looks for an active wrap environment (_active_wrap_env/
_wrap_env_excluding). If one defines the matching wrap hook, it seeds
WrappedAction.Command/Args/Environment/Timeout(+WrappedEnv.Nameor
WrappedStep.Name) from the inner action via_seed_wrapped_action_symbols,
records their EXPR types insymtab.expr_types, and runs the wrap action
through a newrun_wrap_action(...)on both script runners instead of the
innerenter()/run()/exit(). - New runner entry point —
EnvironmentScriptRunner.run_wrap_actionand
StepScriptRunner.run_wrap_actionrun a caller-supplied action against an
already-prepared symbol table. They intentionally do NOT materialize embedded
files or evaluatelet(the caller owns symtab prep) — a documented first-cut
limitation mirroring openjd-rs. run_taskgainsstep_name— optional kwarg, threaded into
WrappedStep.Name(defaults to""when not supplied).- Dependency bump —
openjd-model >= 0.11,< 0.12for the EXPR/WRAP_ACTIONS
bindings.
Risk shape: all new behavior is gated — let only fires when the script has
a let field (only present under the EXPR extension), and wrap dispatch only
fires when an active environment declares a wrap hook (gated by the model's
WRAP_ACTIONS validator). Templates that opt into neither are unaffected. The
sharp edges are: (1) the int(inner_action.timeout) cast, which is wrong when
timeout is a FormatString (FEATURE_BUNDLE_1); (2) _apply_let_bindings
re-parses RHS strings in Python instead of calling the model's already-exposed
Rust evaluate_let_bindings, a parity-drift risk; (3) EnvironmentScriptRunner. run_wrap_action sets self._action = action so cancel targets the wrap
action's cancelation, while the step runner does not.
──BOUNDARY──▶ marks a call crossing from Python into compiled Rust (via the
openjd-model PyO3 bindings); ◀── marks the return.
Flow 1 — run_task with an active wrap environment (onWrapTaskRun)
Entry: a caller invokes Session.run_task(step_script=..., step_name="MyStep")
while a wrap environment (declaring onWrapTaskRun) is active.
Flow: Session.run_task(step_script, task_parameter_values, step_name)
1. _session.py:855 run_task(...)
2. :895 symtab = self._symbol_table(step_script.revision, task_params)
3. :897 action_env_vars = self._evaluate_current_session_env_vars(os_env_vars)
4. :898 self._materialize_path_mapping(rev, action_env_vars, symtab)
5. :904 self._apply_let_bindings(symtab, step_script.let, rev)
6. └─ :1184 _apply_let_bindings — for each "name = rhs":
7. :1229 ArgString_2023_09("{{ "+rhs+" }}", context=EXPR-forced) # re-parse in Python
8. :1233 ──BOUNDARY──▶ expression.evaluate(symtab, path_format) ◀── native typed value
9. :1233 symtab[name] = <value> # NOTE: expr_types NOT recorded here (cf. Flow 3)
10. :909 wrap_env = self._active_wrap_env()
11. └─ :1349 reversed(self._environments_entered); first env w/ a wrap hook
12. :912 on_wrap_task = wrap_env.script.actions.onWrapTaskRun
13. :914 self._seed_wrapped_action_symbols(symtab, inner_action=step.actions.onRun,
14. wrapped_step_name=step_name or "")
15. └─ :1526 _seed_wrapped_action_symbols
16. :1561 ──BOUNDARY──▶ inner_action.command.resolve(symtab=symtab) ◀── str
17. :1564 ──BOUNDARY──▶ arg.resolve(symtab=symtab) for each arg ◀── str (list flattened)
18. :1577 timeout = int(inner_action.timeout) if inner_action.timeout else 0
19. # ⚠ FINDING 1: int() of a FormatString timeout raises ValueError
20. :1581-1604 symtab["WrappedAction.*"] = ...; symtab.expr_types[...] = "STRING"/"INT"/...
21. :920 self._runner = StepScriptRunner(..., script=step_script, symtab=symtab)
22. :938 ┌── wrap set ── self._runner.run_wrap_action(wrap_action)
23. └─ _runner_step_script.py:98 run_wrap_action
24. :118 self._run_action(action, self._symtab, default_timeout=None)
25. └─ _runner_base.py:429 _run_action → resolve cmd/args, _run(...)
26. └── else ── self._runner.run() # ordinary onRun path (no wrap)
Key insight: the wrap action runs against self._symtab as seeded — the
wrap action's own {{ WrappedAction.Command }} etc. resolve through the same
Rust EXPR engine, with expr_types telling build_symbol_table to coerce
WrappedAction.Args to a list and .Timeout to an int. Step (24) deliberately
skips embedded-file materialization, so the wrap action cannot reference the
step's Task.File.* — a documented first-cut limit. Note the step runner's
run_wrap_action does NOT set self._action, so a cancel() during a wrapped
task still reads self._script.actions.onRun.cancelation (the inner action's
cancel config) — which differs from the env runner (Flow 2).
Flow 2 — enter_environment with an outer wrap env (onWrapEnvEnter)
Entry: Session.enter_environment(environment=inner_env) while an outer wrap
environment declaring onWrapEnvEnter is already entered.
Flow: Session.enter_environment(environment)
1. _session.py:608 enter_environment(...)
2. :650 self._environments[identifier] = environment; _environments_entered.append
3. :653 symtab = self._symbol_table(environment.revision)
4. :661 self._apply_let_bindings(symtab, env_script.let, rev) # Flow 1 steps 6-9
5. :665-688 resolve environment.variables → SimplifiedEnvironmentVariableChanges
6. :692 action_env_vars = self._evaluate_current_session_env_vars(...)
7. :694 self._materialize_path_mapping(rev, action_env_vars, symtab)
8. :702 if environment.script.actions.onEnter is not None:
9. :703 wrap_env = self._wrap_env_excluding(identifier) # never self-wrap
10. └─ :1364 reversed(entered), skip self_id, first w/ wrap hook
11. :705 on_wrap_enter = wrap_env.script.actions.onWrapEnvEnter
12. :707 self._seed_wrapped_action_symbols(symtab,
13. inner_action=environment.script.actions.onEnter,
14. wrapped_env_name=environment.name) # Flow 1 steps 15-20
15. :726 self._runner = EnvironmentScriptRunner(..., symtab=symtab)
16. :734 ┌── wrap set ── self._runner.run_wrap_action(wrap_action)
17. └─ _runner_env_script.py:145 run_wrap_action
18. :166 self._action = action # ⚠ sets self._action (cf. step runner)
19. :167 self._run_action(action, self._symtab, default_timeout=None)
20. └── else ── self._runner.enter()
Key insight: _wrap_env_excluding(identifier) is what enforces "an
environment's own onEnter is never wrapped by its own hooks" — the wrap env's
own onEnter (step 8 when it entered itself earlier) found no other wrap env and
ran normally. Unlike the step runner, the env runner's run_wrap_action sets
self._action = action (line 166), so a cancel during a wrapped onEnter uses the
wrap action's cancelation config, not the inner onEnter's. Worth confirming
that's intended (the wrap action is the process actually running, so this is
arguably correct — but it's an asymmetry with the step runner worth a comment).
Flow 3 — _seed_wrapped_action_symbols vs _apply_let_bindings (expr_types)
_seed_wrapped_action_symbols (_session.py:1526)
└─ records symtab.expr_types["WrappedAction.Command"]="STRING", .Args="LIST[STRING]",
.Environment="LIST[STRING]", .Timeout="INT" (+ WrappedEnv/WrappedStep.Name="STRING")
so the Rust build_symbol_table coerces them on the wrap action's resolve.
_apply_let_bindings (_session.py:1184)
└─ symtab[name] = expression.evaluate(...) # stores native value
but does NOT add an entry to symtab.expr_types for `name`.
Key insight: the two seeding paths treat expr_types differently. Wrap
seeding registers types so coercion is exact; let seeding relies on the native
value already carrying its type via evaluate(). This is probably fine for
let because evaluate() returns a real typed value (not a stringified one),
but it's worth confirming a let-bound list re-resolves correctly when later
referenced in an arg {{ }} that expects list flattening — i.e. that the
absence of an expr_types tag doesn't make build_symbol_table re-infer a
different type. (See Reference table.)
Reference — behavior vs the openjd-rs implementation
| Concern | Python (this PR) | Rust (openjd-rs/crates/openjd-sessions/src/session.rs) |
Parity? |
|---|---|---|---|
| Wrap-env selection | _active_wrap_env / _wrap_env_excluding, innermost-wins |
active_wrap_env / wrap_env_excluding, identical traversal |
✅ matches |
WrappedAction.Timeout |
int(inner_action.timeout) (raises on FormatString) |
resolve_action_timeout(...) resolves the FormatString then parses |
⚠ diverges |
let evaluation |
re-parses RHS as ArgString("{{ rhs }}") in Python |
evaluate_let_bindings(...) (one Rust call) — also exposed to Python as openjd._openjd_rs.evaluate_let_bindings |
⚠ reimplements |
let timing vs embedded files |
before file materialization (documented limit) | step runner evaluates let first, then materializes files |
✅ matches (intentional first-cut) |
WrappedAction.Environment |
_openjd_defined_env() excludes host vars |
session_env_vars (openjd_env exports only), host excluded |
✅ matches |
| Args list flattening | str(element) per list/tuple element |
elem.to_display_string() per list element |
⚠ verify coercion match |
Review fixes on top of the RFC 0008 wrap-actions commit: - enter_environment/exit_environment no longer raise a raw ExpressionError out of the public API when an extra `let` binding fails to evaluate: the action fails through _fail_action_before_start with the callback, leaving the environment entered-but-failed exactly like a failed onEnter subprocess. - enter_environment(step_name=...) seeds Step.Name (RFC 0007 §7.3.1) before the extra bindings evaluate, remembered per environment and re-seeded at exit — openjd-rs parity (the Rust runtime threads the per-step resolved symtab into both enter and exit). - let-binding RHS parsing memoized and single-sourced via openjd.model.evaluate_let_bindings (was re-parsed through the Rust engine per env-enter/exit/task). - Wrap environment embedded-file paths allocated once per environment and reused across wrap-hook invocations (stable Env.File.*, no O(task-count) temp-file growth); contents still re-resolved per task. - One resolve_optional_int_field(ge=, le=) replaces three disagreeing optional-int resolvers — this also adds the previously missing non-positive check to Session._resolve_action_timeout (openjd-rs parity). - Complexity/duplication reductions: _fail_action, shared runner cancel() via _cancel_with_effective_cancelation, _make_env_script_runner, _try_inject_wrapped_symbols, WRAP_HOOK_ACTION_NAMES single-sourced, SimplifiedEnvironmentVariableChanges.effective_items() (no more private poking from _collect_session_env_list), whole-field detection via FormatString.whole_field_expression(). - pyproject: note that the openjd-model pin floor must be the first release containing model PR OpenJobDescription#318's surface. Tests: 647 passed (12 new regression tests: failure paths, Step.Name enter+exit, parse memoization, int-field bounds, wrap-file reuse); pre-existing environment failures unchanged. mypy/black/ruff clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Review fixes on top of the RFC 0008 wrap-actions commit: - enter_environment/exit_environment no longer raise a raw ExpressionError out of the public API when an extra `let` binding fails to evaluate: the action fails through _fail_action_before_start with the callback, leaving the environment entered-but-failed exactly like a failed onEnter subprocess. - enter_environment(step_name=...) seeds Step.Name (RFC 0007 §7.3.1) before the extra bindings evaluate, remembered per environment and re-seeded at exit — openjd-rs parity (the Rust runtime threads the per-step resolved symtab into both enter and exit). - let-binding RHS parsing memoized and single-sourced via openjd.model.evaluate_let_bindings (was re-parsed through the Rust engine per env-enter/exit/task). - Wrap environment embedded-file paths allocated once per environment and reused across wrap-hook invocations (stable Env.File.*, no O(task-count) temp-file growth); contents still re-resolved per task. - One resolve_optional_int_field(ge=, le=) replaces three disagreeing optional-int resolvers — this also adds the previously missing non-positive check to Session._resolve_action_timeout (openjd-rs parity). - Complexity/duplication reductions: _fail_action, shared runner cancel() via _cancel_with_effective_cancelation, _make_env_script_runner, _try_inject_wrapped_symbols, WRAP_HOOK_ACTION_NAMES single-sourced, SimplifiedEnvironmentVariableChanges.effective_items() (no more private poking from _collect_session_env_list), whole-field detection via FormatString.whole_field_expression(). - pyproject: note that the openjd-model pin floor must be the first release containing model PR OpenJobDescription#318's surface. Tests: 647 passed (12 new regression tests: failure paths, Step.Name enter+exit, parse memoization, int-field bounds, wrap-file reuse); pre-existing environment failures unchanged. mypy/black/ruff clean. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
1 participant
Implement the RFC 0008 WRAP_ACTIONS runtime in the pure-Python (v0) session: dispatch onWrapEnvEnter/onWrapTaskRun/onWrapEnvExit in enter_environment/run_task/exit_environment, seed
WrappedAction.*/WrappedEnv.Name/WrappedStep.Name and resolve them through the model's Rust EXPR bindings, and add run_wrap_action to both script runners (plus an optional step_name kwarg on run_task). Includes end-to-end tests for all three hooks and the no-wrap path.
Pin openjd-model >= 0.11,< 0.12 -- the model release that ships the EXPR (RFC 0005-0007) + WRAP_ACTIONS (RFC 0008) bindings (build_symbol_table, job_parameter_type_expr_spec) and the wrap-hook model.
The Rust-backed openjd.sessions._v1 implementation (and its test tree) is intentionally excluded from this PR and tracked separately.
Fixes:
What was the problem/requirement? (What/Why)
What was the solution? (How)
What is the impact of this change?
How was this change tested?
See DEVELOPMENT.md for information on running tests.
Was this change documented?
Is this a breaking change?
A breaking change is one that modifies a public contract in a way that is not backwards compatible. See the
Public Interfaces section
of the DEVELOPMENT.md for more information on the public contracts.
If so, then please describe the changes that users of this package must make to update their scripts, or Python applications.
Does this change impact security?
Cross-port to openjd-rs
This package is being migrated to Rust in
openjd-rs/crates/openjd-sessions.Behavioral changes made here should be replicated there to keep the two
implementations in sync until the migration is complete.
openjd-rs(link the PR here): , oropenjd-rsto port this change (link here): , orBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.