Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ classifiers = [
"Topic :: Software Development :: Libraries"
]
dependencies = [
"openjd-model >= 0.9,< 0.10",
# Requires the openjd-model release that ships the EXPR + WRAP_ACTIONS
# bindings (build_symbol_table, job_parameter_type_expr_spec) and the
# wrap-hook model. Those are added by feat: commits on top of model 0.10.0,
# which the model's conventional-commits release tooling bumps to 0.11.0.
"openjd-model >= 0.11,< 0.12",
"pywin32 >= 307; platform_system == 'Windows'",
"psutil >= 5.9,< 7.3; platform_system == 'Windows'",
]
Expand Down
25 changes: 25 additions & 0 deletions src/openjd/sessions/_runner_env_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,31 @@ def _run_env_action(
self._action = action

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review — PR #318: feat: v0 WRAP_ACTIONS runtime and EXPR integration (RFC 0005–0008)

Repo: OpenJobDescription/openjd-sessions-for-python · wrap-actions-v0mainline
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:1577int(inner_action.timeout) raises on a FormatString timeout (FEATURE_BUNDLE_1). (PLAUSIBLE)

timeout = int(inner_action.timeout) if inner_action.timeout else 0

Action.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. resolve inner_action.timeout via the
    FormatString path when it's a FormatString, fall back to int(...) only for
    the already-numeric case. Mirror resolve_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.rsopenjd_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 let semantics will
    drift from the canonical Rust one (binding scope/ordering, error messages, the
    name = expr split, profile/function-library handling). The Rust path takes the
    whole bindings list and the function library in one call; the Python loop does
    a naive partition("=") that, e.g., would mishandle an = inside the RHS
    expression differently than the model's parse_let_bindings validator. 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
    the expr_types handling (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 let binds 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's expr_types field exists to prevent. The chained-int test passes, but
    there's no test for a let-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 a let-bound
    list referenced from args.

4. _runner_env_script.py:166 vs _runner_step_script.pyrun_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's run_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_types directly (it's always a dict) and drop
    the type: ignore. Minor, but it removes a "this attribute might not exist"
    signal that's no longer true.

6. _session.py enter_environment/exit_environmentgetattr(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_excluding for never-self-wrap,
    and _openjd_defined_env() excluding host-inherited variables — all match
    session.rs line-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 on run_wrap_action is documented in both runners and
    matches the Rust run_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 chained let ordering.
  • The expr_types discipline in _seed_wrapped_action_symbols shows correct
    understanding of the model's coercion contract.

Refuted / considered but not flagged

  • _active_wrap_env after pop in exit_environment — the env is popped from
    _environments_entered before _active_wrap_env() is called, so it correctly
    returns the outer wrap env. Matches the Rust comment at session.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 Rust to_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 (a let/wrap list of bools would be the test).

Suggested next steps (only if you want them)

  1. Fix Finding 1 (timeout) and Finding 2 (reuse the Rust evaluate_let_bindings)
    — these are the two with real behavioral weight.
  2. Add the two regression tests called out (FormatString timeout in a wrapped
    action; let-bound list consumed by an arg).
  3. Add a conformance fixture for the timeout-resolution behavior in
    openjd-specifications once Finding 1 is fixed (parity rule).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

  • let bindings (RFC 0007)enter_environment / exit_environment /
    run_task now call a new Session._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.Name or
    WrappedStep.Name) from the inner action via _seed_wrapped_action_symbols,
    records their EXPR types in symtab.expr_types, and runs the wrap action
    through a new run_wrap_action(...) on both script runners instead of the
    inner enter()/run()/exit().
  • New runner entry pointEnvironmentScriptRunner.run_wrap_action and
    StepScriptRunner.run_wrap_action run a caller-supplied action against an
    already-prepared symbol table. They intentionally do NOT materialize embedded
    files or evaluate let (the caller owns symtab prep) — a documented first-cut
    limitation mirroring openjd-rs.
  • run_task gains step_name — optional kwarg, threaded into
    WrappedStep.Name (defaults to "" when not supplied).
  • Dependency bumpopenjd-model >= 0.11,< 0.12 for 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

self._run_action(self._action, symtab, default_timeout=default_timeout)

def run_wrap_action(
self,
action: ActionModel,
*,
default_timeout: Optional[timedelta] = None,
) -> None:
"""Run a caller-supplied wrap action (RFC 0008) against this runner's
prepared symbol table.

This is the low-level entry point used by the Session's WRAP_ACTIONS
dispatch: the Session chooses the action (an outer environment's
``onWrapEnvEnter`` / ``onWrapEnvExit``) and seeds the symbol table with
``WrappedAction.*`` / ``WrappedEnv.*`` before calling.

Unlike :meth:`enter` / :meth:`exit`, this does NOT materialize any
embedded files or evaluate let bindings — the caller is responsible for
preparing the symbol table. This mirrors the Rust
``EnvironmentScriptRunner::run_wrap_action`` first-cut behavior.
"""
if self.state != ScriptRunnerState.READY:
raise RuntimeError("This cannot be used to run a second subprocess.")
log_subsection_banner(self._logger, "Phase: Setup")
self._action = action
self._run_action(action, self._symtab, default_timeout=default_timeout)

def enter(self) -> None:
"""Run the Environment's onEnter action."""
if self.state != ScriptRunnerState.READY:
Expand Down
25 changes: 24 additions & 1 deletion src/openjd/sessions/_runner_step_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
TerminateCancelMethod,
)
from ._session_user import SessionUser
from ._types import ActionState, StepScriptModel
from ._types import ActionModel, ActionState, StepScriptModel

__all__ = ("StepScriptRunner",)

Expand Down Expand Up @@ -95,6 +95,29 @@ def __init__(
if not isinstance(self._script, StepScript_2023_09):
raise NotImplementedError("Unknown model type")

def run_wrap_action(
self,
action: ActionModel,
*,
default_timeout: Optional[timedelta] = None,
) -> None:
"""Run a caller-supplied wrap action (RFC 0008) against this runner's
prepared symbol table.

Used by the Session's WRAP_ACTIONS dispatch when an active wrap
environment defines ``onWrapTaskRun``: the Session seeds the symbol
table with ``WrappedAction.*`` / ``WrappedStep.Name`` and supplies the
wrap action here, which runs instead of the step's own ``onRun``.

Unlike :meth:`run`, this does NOT materialize the step's embedded files
or evaluate let bindings — the caller prepares the symbol table. This
mirrors the Rust ``run_wrap_action`` first-cut behavior.
"""
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


def run(self) -> None:
"""Run the Step Script's onRun Action."""
if self.state != ScriptRunnerState.READY:
Expand Down
Loading
Loading