Skip to content

Commit cd5d5b6

Browse files
committed
fix: RFC 0005/0008 typed-arg and null-vs-empty parity with openjd-rs
Two Rust-parity fixes in the v0 session runtime: 1. WrappedAction.Args now uses RFC 0005 1.3.2 typed argument semantics. The enforcement path's typed arg loop (null skip, list flattening, display coercion) is extracted into a shared module-level helper, resolve_action_arg_values, in _runner_base.py; both _inject_wrapped_task_symbols and _inject_wrapped_env_symbols now use it, so a wrap hook sees exactly the argv the wrapped action would have run with unwrapped -- mirroring openjd-rs, whose seed_wrapped_action_symbols resolves through the same resolve_action_args as the runner. The helper also gains openjd-rs's plain-string-resolution fallback when typed resolution fails. 2. An empty string is no longer conflated with null. resolve_optional_int_field and resolve_effective_cancelation's deferred-mode branch now resolve typed (resolve_value) and treat only a typed null result as "field omitted" / "no cancelation declared". A genuine empty string now reaches the "must be a positive integer, got ''" / "must resolve to ... got ''" errors, matching openjd-rs's resolve_action_timeout, resolve_notify_period_seconds, and resolve_effective_cancelation, which only special-case ExprValue::Null. The whole_field_expression() pre-check is removed: resolve_value only yields a typed null for whole-field expressions, so null semantics remain whole-field-only by construction. Test updates: the deferred-cancelation unit-test helper now parses its format strings with the EXPR extension (deferred forwarding is an RFC 0008 construct and WRAP_ACTIONS requires EXPR), since the previous legacy parse pinned the non-Rust "" == null behavior. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
1 parent 1a5f8fc commit cd5d5b6

10 files changed

Lines changed: 454 additions & 141 deletions

src/openjd/sessions/_embedded_files.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ def allocate_file_paths(
220220
the file contents.
221221
222222
Splitting allocation from :meth:`write_file_contents` lets the runner
223-
evaluate EXPR ``let`` bindings between the two phases (RFC 0007): a
223+
evaluate EXPR ``let`` bindings between the two phases (RFC 0005): a
224224
file's *path* never depends on ``let`` values (``filename`` is a plain
225225
string), so the ``Env.File.*``/``Task.File.*`` symbols are available
226226
to the bindings, while a file's ``data`` is written afterwards so it

src/openjd/sessions/_runner_base.py

Lines changed: 153 additions & 85 deletions
Large diffs are not rendered by default.

src/openjd/sessions/_runner_env_script.py

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
from typing import Callable, Optional
77

88
from openjd.model import SymbolTable
9-
from openjd.model.v2023_09 import Action as Action_2023_09
109
from openjd.model.v2023_09 import EnvironmentScript as EnvironmentScript_2023_09
1110
from ._embedded_files import EmbeddedFilesScope, _FileRecord
1211
from ._logging import log_subsection_banner
@@ -141,9 +140,7 @@ def _run_env_action(
141140
log_subsection_banner(self._logger, "Phase: Setup")
142141

143142
let_bindings = (
144-
getattr(self._environment_script, "let", None)
145-
if self._environment_script is not None
146-
else None
143+
self._environment_script.let if self._environment_script is not None else None
147144
)
148145
# Write any embedded files to disk. File paths are allocated before
149146
# the script's EXPR `let` bindings evaluate (so bindings can reference
@@ -174,7 +171,12 @@ def _run_env_action(
174171

175172
# Construct the command by evalutating the format strings in the command
176173
self._action = action
177-
self._run_action(self._action, symtab, default_timeout=default_timeout)
174+
self._run_action(
175+
self._action,
176+
symtab,
177+
default_timeout=default_timeout,
178+
default_notify_period_seconds=ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS,
179+
)
178180

179181
def enter(self) -> None:
180182
"""Run the Environment's onEnter action."""
@@ -268,13 +270,8 @@ def cancel(
268270
# Nothing to do.
269271
return
270272

271-
# For the type checker
272-
assert isinstance(self._action, Action_2023_09)
273-
274-
self._cancel_with_effective_cancelation(
275-
cancelation=self._action.cancelation,
276-
symtab=self._symtab,
277-
default_notify_period_seconds=ENV_ACTION_DEFAULT_NOTIFY_PERIOD_SECONDS,
278-
time_limit=time_limit,
279-
mark_action_failed=mark_action_failed,
280-
)
273+
# Cancel with the effective method resolved at launch time by
274+
# _run_action, against the action's own final scope (the script's
275+
# lets and Env.File.* symbols, plus WrappedAction.* for a wrap
276+
# hook) — openjd-rs parity.
277+
self._cancel_with_resolved_method(time_limit, mark_action_failed)

src/openjd/sessions/_runner_step_script.py

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ def run(self) -> None:
9797

9898
# For the type checker.
9999
assert isinstance(self._script, StepScript_2023_09)
100-
let_bindings = getattr(self._script, "let", None)
100+
let_bindings = self._script.let
101101
# Write any embedded files to disk. File paths are allocated before
102102
# the script's EXPR `let` bindings evaluate (so bindings can reference
103103
# Task.File.*), and contents are written after (so `data` can
@@ -121,18 +121,16 @@ def run(self) -> None:
121121
symtab = self._symtab
122122

123123
# Construct the command by evalutating the format strings in the command
124-
self._run_action(self._script.actions.onRun, symtab)
124+
self._run_action(
125+
self._script.actions.onRun,
126+
symtab,
127+
default_notify_period_seconds=TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS,
128+
)
125129

126130
def cancel(
127131
self, *, time_limit: Optional[timedelta] = None, mark_action_failed: bool = False
128132
) -> None:
129-
# For the type checker.
130-
assert isinstance(self._script, StepScript_2023_09)
131-
132-
self._cancel_with_effective_cancelation(
133-
cancelation=self._script.actions.onRun.cancelation,
134-
symtab=self._symtab,
135-
default_notify_period_seconds=TASK_RUN_DEFAULT_NOTIFY_PERIOD_SECONDS,
136-
time_limit=time_limit,
137-
mark_action_failed=mark_action_failed,
138-
)
133+
# Cancel with the effective method resolved at launch time by
134+
# _run_action, against the action's own final scope (its lets and
135+
# Task.File.* symbols) — openjd-rs parity.
136+
self._cancel_with_resolved_method(time_limit, mark_action_failed)

src/openjd/sessions/_session.py

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
from ._runner_base import (
4646
ScriptRunnerBase,
4747
apply_let_bindings,
48+
resolve_action_arg_values,
4849
resolve_effective_cancelation,
4950
resolve_optional_int_field,
5051
)
@@ -373,7 +374,7 @@ def __init__(
373374
ParameterValue (a dataclass containing the type and value of the parameter)
374375
job_name (Optional[str]): The resolved name of the Job this Session belongs to.
375376
When provided, it is seeded as the ``Job.Name`` template variable
376-
(RFC 0007 §7.3.1, EXPR extension) for the session's actions —
377+
(RFC 0005; Template Schemas §7.3.1, EXPR extension) for the session's actions —
377378
mirroring the Job.Name symbol that openjd-rs carries in its
378379
session symbol tables. Defaults to None (no Job.Name symbol).
379380
path_mapping_rules (Optional[list[PathMappingRule]]): A list of the path mapping rules to apply
@@ -424,7 +425,7 @@ def __init__(
424425
# environment exits so its onExit resolves in the same scope.
425426
self._environment_extra_let_bindings: dict[EnvironmentIdentifier, list[str]] = dict()
426427
# The owning step's name supplied when an environment was entered
427-
# (seeds Step.Name, RFC 0007 EXPR), re-seeded when the environment
428+
# (seeds Step.Name, RFC 0005 EXPR), re-seeded when the environment
428429
# exits so the re-applied extra `let` bindings resolve in the same
429430
# scope.
430431
self._environment_step_names: dict[EnvironmentIdentifier, str] = dict()
@@ -711,15 +712,15 @@ def enter_environment(
711712
Key: Environment variable name
712713
Value: Value for the environment variable.
713714
extra_let_bindings (Optional[list[str]]): Additional EXPR ``let``
714-
bindings (RFC 0007) evaluated into the symbol table before the
715+
bindings (RFC 0005) evaluated into the symbol table before the
715716
environment's variables and actions resolve. A step's
716717
environments are entered with the step-level ``let`` bindings
717718
(``Step.let`` on the instantiated Job) so both can reference
718719
them — the v0 counterpart of the per-step resolved symbol
719720
table that openjd-rs threads into enter_environment.
720721
step_name (Optional[str]): The name of the step whose
721722
stepEnvironments are being entered, if any. Seeds
722-
``Step.Name`` (RFC 0007 EXPR) into the symbol table before
723+
``Step.Name`` (RFC 0005 EXPR) into the symbol table before
723724
the extra ``let`` bindings evaluate, so step-level bindings
724725
and the environment's variables and actions can reference
725726
it — openjd-rs threads a per-step resolved symbol table into
@@ -767,14 +768,14 @@ def enter_environment(
767768

768769
symtab = self._symbol_table(environment.revision)
769770

770-
# RFC 0007 §7.3.1 (EXPR): the owning step's name. Only EXPR templates
771+
# RFC 0005; Template Schemas §7.3.1 (EXPR): the owning step's name. Only EXPR templates
771772
# pass validation referencing Step.Name, so seeding it when known does
772773
# not change non-EXPR behavior. Seeded before the extra `let` bindings
773774
# evaluate so a step-level binding may reference it.
774775
if step_name is not None:
775776
symtab["Step.Name"] = step_name
776777

777-
# Step-level `let` bindings (RFC 0007) accompany a step's
778+
# Step-level `let` bindings (RFC 0005) accompany a step's
778779
# environments: evaluate them first so the environment's variables
779780
# and actions can reference them.
780781
if extra_let_bindings:
@@ -796,7 +797,7 @@ def enter_environment(
796797
)
797798
return identifier
798799

799-
# Note: the environment script's own EXPR `let` bindings (RFC 0007)
800+
# Note: the environment script's own EXPR `let` bindings (RFC 0005)
800801
# are evaluated by the script runner, after embedded-file paths are
801802
# allocated (so bindings can reference Env.File.*). The environment's
802803
# `variables` resolve against the session symbol table without them,
@@ -977,9 +978,9 @@ def exit_environment(
977978
symtab = self._symbol_table(environment.revision)
978979
self._materialize_path_mapping(environment.revision, action_env_vars, symtab)
979980

980-
# Re-seed the owning step's name (Step.Name, RFC 0007 EXPR) and
981+
# Re-seed the owning step's name (Step.Name, RFC 0005 EXPR) and
981982
# re-apply the extra `let` bindings this environment was entered with
982-
# (e.g. the owning step's step-level bindings, RFC 0007) so its onExit
983+
# (e.g. the owning step's step-level bindings, RFC 0005) so its onExit
983984
# resolves in the same scope as its onEnter.
984985
exit_step_name = self._environment_step_names.pop(identifier, None)
985986
if exit_step_name is not None:
@@ -999,7 +1000,7 @@ def exit_environment(
9991000
)
10001001
return
10011002

1002-
# Note: the environment script's own EXPR `let` bindings (RFC 0007)
1003+
# Note: the environment script's own EXPR `let` bindings (RFC 0005)
10031004
# are evaluated by the script runner (after embedded-file path
10041005
# allocation); the wrap-interception branch below evaluates them
10051006
# itself before resolving the wrapped onExit.
@@ -1116,15 +1117,15 @@ def run_task(
11161117

11171118
self._reset_action_state()
11181119
symtab = self._symbol_table(step_script.revision, task_parameter_values)
1119-
# RFC 0007 §7.3.1 (EXPR): the running step's name. Only EXPR templates
1120+
# RFC 0005; Template Schemas §7.3.1 (EXPR): the running step's name. Only EXPR templates
11201121
# pass validation referencing Step.Name, so seeding it when known does
11211122
# not change non-EXPR behavior.
11221123
if step_name is not None:
11231124
symtab["Step.Name"] = step_name
11241125
action_env_vars = self._evaluate_current_session_env_vars(os_env_vars)
11251126
self._materialize_path_mapping(step_script.revision, action_env_vars, symtab)
11261127

1127-
# Note: the step script's EXPR `let` bindings (RFC 0007) are evaluated
1128+
# Note: the step script's EXPR `let` bindings (RFC 0005) are evaluated
11281129
# by the script runner, after embedded-file paths are allocated (so
11291130
# bindings can reference Task.File.*). The wrap-interception branch
11301131
# below evaluates them itself before resolving the wrapped onRun.
@@ -1237,7 +1238,7 @@ def _run_task_without_session_env(
12371238

12381239
self._materialize_path_mapping(step_script.revision, action_env_vars, symtab)
12391240

1240-
# Note: the step script's EXPR `let` bindings (RFC 0007) are evaluated
1241+
# Note: the step script's EXPR `let` bindings (RFC 0005) are evaluated
12411242
# by the script runner, after embedded-file paths are allocated.
12421243

12431244
self._runner = StepScriptRunner(
@@ -1461,7 +1462,7 @@ def record_expr_types(
14611462
symtab[working_dir_key] = str(self.working_directory)
14621463
# Session.WorkingDirectory is a host-format path value in openjd-rs.
14631464
symtab.expr_types[working_dir_key] = ParameterValueType.PATH.value
1464-
# RFC 0007 §7.3.1 (EXPR): the job's resolved name. Only templates
1465+
# RFC 0005; Template Schemas §7.3.1 (EXPR): the job's resolved name. Only templates
14651466
# declaring EXPR pass validation referencing Job.Name, so seeding
14661467
# it whenever known does not change non-EXPR behavior.
14671468
if self._job_name is not None:
@@ -1654,8 +1655,8 @@ def _try_inject_wrapped_symbols(
16541655
try:
16551656
inner_symtab = self._build_wrapped_inner_scope(
16561657
scope,
1657-
getattr(inner_script, "let", None) if inner_script is not None else None,
1658-
getattr(inner_script, "embeddedFiles", None) if inner_script is not None else None,
1658+
inner_script.let if inner_script is not None else None,
1659+
inner_script.embeddedFiles if inner_script is not None else None,
16591660
symtab,
16601661
)
16611662
inject(inner_symtab)
@@ -1771,9 +1772,13 @@ def _inject_wrapped_env_symbols(
17711772
is removed from tracking, so the wrapped environment's own
17721773
variables are included."""
17731774
command = inner_action.command.resolve(symtab=inner_symtab)
1774-
args = (
1775-
[a.resolve(symtab=inner_symtab) for a in inner_action.args] if inner_action.args else []
1776-
)
1775+
# RFC 0005 §1.3.2 typed argument semantics (null skip, list
1776+
# flattening), shared with the enforcement path
1777+
# (ScriptRunnerBase._run_action) so the hook sees exactly the
1778+
# arguments the wrapped action would have run with unwrapped —
1779+
# mirroring openjd-rs's seed_wrapped_action_symbols, which resolves
1780+
# via the same resolve_action_args as the runner.
1781+
args = resolve_action_arg_values(inner_action.args, inner_symtab)
17771782
symtab["WrappedAction.Command"] = command
17781783
symtab["WrappedAction.Args"] = args
17791784
symtab["WrappedAction.Environment"] = (
@@ -1807,9 +1812,10 @@ def _inject_wrapped_task_symbols(
18071812
on_run = step_script.actions.onRun
18081813

18091814
symtab["WrappedAction.Command"] = on_run.command.resolve(symtab=inner_symtab)
1810-
symtab["WrappedAction.Args"] = (
1811-
[arg.resolve(symtab=inner_symtab) for arg in on_run.args] if on_run.args else []
1812-
)
1815+
# RFC 0005 §1.3.2 typed argument semantics (null skip, list
1816+
# flattening), shared with the enforcement path — see
1817+
# _inject_wrapped_env_symbols.
1818+
symtab["WrappedAction.Args"] = resolve_action_arg_values(on_run.args, inner_symtab)
18131819
symtab["WrappedAction.Environment"] = self._collect_session_env_list()
18141820
symtab["WrappedAction.Timeout"] = self._resolve_action_timeout(on_run, inner_symtab)
18151821
self._inject_wrapped_cancelation_symbols(
@@ -1898,7 +1904,7 @@ def _materialize_path_mapping(
18981904
else:
18991905
rules_dict = dict()
19001906
symtab[ValueReferenceConstants_2023_09.HAS_PATH_MAPPING_RULES.value] = "false"
1901-
# RFC 0007 §7.3: for EXPR evaluation Session.HasPathMappingRules is a
1907+
# RFC 0005; Template Schemas §7.3: for EXPR evaluation Session.HasPathMappingRules is a
19021908
# boolean and Session.PathMappingRulesFile is a path, matching
19031909
# openjd-rs's typed session symbols. The legacy (non-EXPR)
19041910
# interpolation path ignores these types and keeps the string forms.

test/openjd/sessions_v0/test_runner_env_script.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,9 @@ def test_cancel(
387387
# The lower-level process runners have been thoroughly tested for cancel's
388388
# functionality, so this seems fine.
389389

390-
with patch.object(EnvironmentScriptRunner, "_run_action"):
390+
# Patch _run (not _run_action): the effective cancel method is now
391+
# resolved by _run_action at launch time and consumed by cancel().
392+
with patch.object(EnvironmentScriptRunner, "_run"):
391393
with patch.object(EnvironmentScriptRunner, "_cancel") as mock_cancel:
392394
# GIVEN
393395
script = EnvironmentScript_2023_09(
@@ -468,6 +470,7 @@ def test_run_env_action_passes_default_timeout(
468470
action,
469471
symtab,
470472
default_timeout=default_timeout,
473+
default_notify_period_seconds=30,
471474
)
472475

473476
def test_exit_uses_default_timeout(

test/openjd/sessions_v0/test_runner_step_script.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,9 @@ def test_cancel(
252252
# The lower-level process runners have been thoroughly tested for cancel's
253253
# functionality, so this seems fine.
254254

255-
with patch.object(StepScriptRunner, "_run_action"):
255+
# Patch _run (not _run_action): the effective cancel method is now
256+
# resolved by _run_action at launch time and consumed by cancel().
257+
with patch.object(StepScriptRunner, "_run"):
256258
with patch.object(StepScriptRunner, "_cancel") as mock_cancel:
257259
# GIVEN
258260
script = StepScript_2023_09(

0 commit comments

Comments
 (0)