fix: Defer forwardable action fields to run time; cache key and lint fixes

leongdl · leongdl · commit 914ea051592f · 2026-07-23T23:40:45.000-07:00
Three fixes:

1. Defer run-time-resolvable action fields to run time, matching
openjd-rs (job/create_job/instantiate.rs clones timeout and cancelation
unresolved into the Job; the session resolves them right before the
action runs). Previously Action.timeout and the cancelation classes'
notifyPeriodInSeconds were in resolve_fields, so create_job resolved
them at job creation. A wrap hook forwarding
"{{WrappedAction.Timeout}}" or
"{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}" (RFC 0008
round-trip forwarding) decoded cleanly but crashed create_job with
"Undefined variable" because those symbols only exist at run time.
The fields now carry their raw FormatStrings through job instantiation;
validation still happens at TEMPLATE scope at decode time (unchanged,
matching openjd-rs format_strings.rs).

Deliberate behavior change: a plain FEATURE_BUNDLE_1 template with
timeout "{{Param.X}}" now carries the FormatString into the Job instead
of resolving it at creation — resolution moves to the session, as in
openjd-rs. The sessions runtime already resolves FormatString
timeouts/notify periods at run time (_resolve_action_timeout,
resolve_effective_cancelation).

Fixes the wrap-cancelation-roundtrip-job-environment conformance
fixture, which failed at create_job for exactly this bug.

2. Fix a typed/untyped cache-key collision in symtab_to_expr_values:
priming the engine-table cache with types=None on a symbol table whose
expr_types is non-empty cached an untyped table that a later typed call
at the same mutation version was served, losing the type coercions.
The cache condition is now strict: cacheable iff types is the symtab's
own expr_types, or types is None and the symtab has no expr_types.

3. Module-level Path import in symbol-table cache tests (ruff
F821/F401), retained from the previous revision of this commit.

Signed-off-by: David Leong &lt;116610336+leongdl@users.noreply.github.com&gt;
diff --git a/src/openjd/model/_format_strings/_expr_support.py b/src/openjd/model/_format_strings/_expr_support.py
@@ -83,10 +83,15 @@ def symtab_to_expr_values(
     embedded-file ``data``) against one unchanged table — without the cache
     the whole table is rebuilt across the Rust boundary per expression.
     Caching requires ``types`` to be the symbol table's own ``expr_types``
-    (or ``None``), which is what the evaluation layer passes; any other
-    ``types`` object bypasses the cache.
+    (which is what the evaluation layer passes), or ``None`` when the symbol
+    table has no ``expr_types``; any other combination bypasses the cache.
+    In particular an untyped (``types=None``) build on a symbol table that
+    *has* ``expr_types`` is never cached — otherwise a later typed call at
+    the same mutation version would be served the stale untyped table and
+    lose the type coercions (typed/untyped cache-key collision).
     """
-    cacheable = types is None or types is getattr(symtab, "expr_types", None)
+    symtab_expr_types = getattr(symtab, "expr_types", None)
+    cacheable = types is symtab_expr_types or (types is None and not symtab_expr_types)
     version = getattr(symtab, "_version", None)
     cache_key = ("engine_table", str(path_format))
     if cacheable and version is not None:
diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py
@@ -344,7 +344,13 @@ class CancelationMethodNotifyThenTerminate(OpenJDModel_v2023_09):
     mode: Literal[CancelationMode.NOTIFY_THEN_TERMINATE]
     notifyPeriodInSeconds: Optional[Union[NotifyPeriodType, FormatString]] = None  # noqa: N815
 
-    _job_creation_metadata = JobCreationMetadata(resolve_fields={"notifyPeriodInSeconds"})
+    # A FormatString notifyPeriodInSeconds is NOT resolved at job creation:
+    # it is carried through unresolved and resolved at run time by the
+    # session, matching openjd-rs (job/create_job/instantiate.rs clones the
+    # cancelation object unresolved into the Job). This is what lets RFC 0008
+    # wrap hooks forward "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}",
+    # whose symbols only exist at run time.
+    _job_creation_metadata = JobCreationMetadata()
 
     @field_validator("notifyPeriodInSeconds", mode="before")
     @classmethod
@@ -432,7 +438,13 @@ class CancelationMethodDeferred(OpenJDModel_v2023_09):
     mode: FormatString
     notifyPeriodInSeconds: Optional[Union[NotifyPeriodType, FormatString]] = None  # noqa: N815
 
-    _job_creation_metadata = JobCreationMetadata(resolve_fields={"notifyPeriodInSeconds"})
+    # Neither `mode` nor a FormatString `notifyPeriodInSeconds` is resolved
+    # at job creation: the whole deferred cancelation object is carried
+    # through unresolved and resolved at run time by the session, matching
+    # openjd-rs (job/create_job/instantiate.rs). Run-time-only symbols such
+    # as WrappedAction.Cancelation.* are how RFC 0008 wrap hooks forward the
+    # wrapped action's cancelation.
+    _job_creation_metadata = JobCreationMetadata()
 
     @field_validator("mode", mode="before")
     @classmethod
@@ -584,16 +596,23 @@ class Action(OpenJDModel_v2023_09):
     timeout: Optional[Union[PositiveInt, FormatString]] = None
     cancelation: Optional[CancelationMethod] = None
 
-    _job_creation_metadata = JobCreationMetadata(resolve_fields={"timeout"})
+    # A FormatString `timeout` is NOT resolved at job creation: it is carried
+    # through job instantiation unresolved and resolved at run time by the
+    # session, matching openjd-rs (job/create_job/instantiate.rs clones
+    # timeout and cancelation unresolved into the Job, and the session
+    # resolves them right before the action runs). This is what lets RFC 0008
+    # wrap hooks forward "{{WrappedAction.Timeout}}" — those symbols only
+    # exist at run time.
+    _job_creation_metadata = JobCreationMetadata()
 
     # `timeout` and `cancelation` (its notifyPeriodInSeconds and a deferred
-    # format-string mode) are plain @fmtstring fields resolved at job
-    # creation, before any session exists — unlike `command`/`args`, which
-    # resolve in the session. They therefore validate at template scope: no
-    # Session.*, no Env.File.*/Task.File.*, and no host-context functions.
-    # The RFC 0008 wrap hooks may still forward the wrapped action's values
-    # ("{{WrappedAction.Timeout}}"): the WrappedAction.* symbols are injected
-    # per-hook at every scope via EnvironmentActions._template_field_inject.
+    # format-string mode) still VALIDATE at template scope: no Session.*, no
+    # Env.File.*/Task.File.*, and no host-context functions (matching
+    # openjd-rs format_strings.rs). The RFC 0008 wrap hooks may forward the
+    # wrapped action's values ("{{WrappedAction.Timeout}}"): the
+    # WrappedAction.* symbols are injected per-hook at every scope via
+    # EnvironmentActions._template_field_inject, and openjd-rs's
+    # symtab-filter preserves them for run time.
     _template_field_scopes = {
         "timeout": ResolutionScope.TEMPLATE,
         "cancelation": ResolutionScope.TEMPLATE,
diff --git a/test/openjd/model_v0/test_symbol_table_cache.py b/test/openjd/model_v0/test_symbol_table_cache.py
@@ -10,6 +10,8 @@
 ``expr_host_rules``) invalidates the cache.
 """
 
+from pathlib import Path
+
 import pytest
 
 from openjd.model import SymbolTable
@@ -68,6 +70,42 @@ def test_expr_types_update_invalidates(self) -> None:
         second = symtab_to_expr_values(symtab, types=symtab.expr_types or None)
         assert first is not second
 
+    def test_untyped_prime_does_not_poison_typed_call(self) -> None:
+        # Regression: priming the cache with types=None on a symbol table
+        # whose expr_types is non-empty must NOT cache an untyped engine
+        # table — a later typed call at the same mutation version would be
+        # served the stale untyped table and lose the type coercions.
+        from openjd.expr import ExprProfile
+
+        from openjd.model._format_strings._expr_support import parse_expr_or_raise
+
+        symtab = SymbolTable()
+        symtab["Param.X"] = "10"
+        symtab.expr_types["Param.X"] = "INT"
+
+        # WHEN: prime with an untyped build on the typed symtab.
+        untyped = symtab_to_expr_values(symtab, types=None)
+
+        # THEN: the subsequent typed call is not served the stale table...
+        typed = symtab_to_expr_values(symtab, types=symtab.expr_types or None)
+        assert typed is not untyped
+
+        # ...and coerces correctly: arithmetic sees the int 10, not "10".
+        parsed = parse_expr_or_raise("Param.X + 1")
+        result = parsed.evaluate(values=typed, profile=ExprProfile.current())
+        assert result.item() == 11
+
+        # The typed build is the one cached for the standard call form.
+        assert symtab_to_expr_values(symtab, types=symtab.expr_types or None) is typed
+
+    def test_untyped_call_still_cached_when_symtab_has_no_types(self) -> None:
+        # types=None on a symtab without expr_types remains cacheable (this
+        # is the evaluation layer's call form for untyped tables).
+        symtab = SymbolTable()
+        symtab["Param.X"] = "10"
+        first = symtab_to_expr_values(symtab, types=symtab.expr_types or None)
+        assert symtab_to_expr_values(symtab, types=symtab.expr_types or None) is first
+
     def test_foreign_types_mapping_bypasses_cache(self) -> None:
         # A caller-supplied types mapping that is not the table's own
         # expr_types must not poison or consult the cache.
@@ -158,12 +196,11 @@ def test_profile_cached_and_invalidated_by_host_rules(self) -> None:
 
 class TestTypedResolutionSingleEvaluation:
     def test_range_expression_evaluated_once_per_definition(
-        self, monkeypatch: pytest.MonkeyPatch, tmp_path: "Path"
+        self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path
     ) -> None:
         """RFC 0006 typed whole-field range resolution: the expression is
         evaluated exactly once per task-parameter definition — the create_as
         target-model decision and the field value share the result."""
-        from pathlib import Path
 
         from openjd.model import create_job, decode_job_template, preprocess_job_parameters
         from openjd.model._internal import _create_job as internal_create_job
diff --git a/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py b/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py
@@ -620,6 +620,94 @@ def test_amount_requirement_min_greater_than_max_resolved_fails(self) -> None:
             )
         assert "max" in str(excinfo.value).lower()
 
+    def test_timeout_and_notify_period_carried_through_unresolved(self) -> None:
+        """timeout and notifyPeriodInSeconds format strings are NOT resolved
+        at job creation: they are carried through the instantiated Job as raw
+        FormatStrings and resolved at run time by the session, matching
+        openjd-rs (job/create_job/instantiate.rs clones timeout+cancelation
+        unresolved into the Job). This is what allows RFC 0008 wrap hooks to
+        forward run-time-only symbols such as WrappedAction.Timeout."""
+        from openjd.model import create_job, decode_job_template
+        from openjd.model._format_strings import FormatString
+        from openjd.model._types import ParameterValue, ParameterValueType
+
+        template = decode_job_template(
+            template={
+                "specificationVersion": "jobtemplate-2023-09",
+                "extensions": ["FEATURE_BUNDLE_1"],
+                "name": "Test",
+                "parameterDefinitions": [{"name": "TO", "type": "INT", "default": 30}],
+                "steps": [
+                    {
+                        "name": "Step1",
+                        "script": {
+                            "actions": {
+                                "onRun": {
+                                    "command": "echo",
+                                    "timeout": "{{Param.TO}}",
+                                    "cancelation": {
+                                        "mode": "NOTIFY_THEN_TERMINATE",
+                                        "notifyPeriodInSeconds": "{{Param.TO}}",
+                                    },
+                                }
+                            }
+                        },
+                    }
+                ],
+            },
+            supported_extensions=[ExtensionName.FEATURE_BUNDLE_1],
+        )
+
+        # WHEN
+        job = create_job(
+            job_template=template,
+            job_parameter_values={"TO": ParameterValue(type=ParameterValueType.INT, value="30")},
+        )
+
+        # THEN: creation succeeds and both fields carry the raw FormatString.
+        on_run = job.steps[0].script.actions.onRun
+        assert isinstance(on_run.timeout, FormatString)
+        assert str(on_run.timeout) == "{{Param.TO}}"
+        assert on_run.cancelation is not None
+        assert isinstance(on_run.cancelation.notifyPeriodInSeconds, FormatString)
+        assert str(on_run.cancelation.notifyPeriodInSeconds) == "{{Param.TO}}"
+
+    def test_literal_timeout_and_notify_period_stay_ints_through_create_job(self) -> None:
+        """Literal integer timeout/notifyPeriodInSeconds are unaffected by
+        the run-time deferral of format-string values."""
+        from openjd.model import create_job, decode_job_template
+
+        template = decode_job_template(
+            template={
+                "specificationVersion": "jobtemplate-2023-09",
+                "extensions": ["FEATURE_BUNDLE_1"],
+                "name": "Test",
+                "steps": [
+                    {
+                        "name": "Step1",
+                        "script": {
+                            "actions": {
+                                "onRun": {
+                                    "command": "echo",
+                                    "timeout": 30,
+                                    "cancelation": {
+                                        "mode": "NOTIFY_THEN_TERMINATE",
+                                        "notifyPeriodInSeconds": 15,
+                                    },
+                                }
+                            }
+                        },
+                    }
+                ],
+            },
+            supported_extensions=[ExtensionName.FEATURE_BUNDLE_1],
+        )
+        job = create_job(job_template=template, job_parameter_values={})
+        on_run = job.steps[0].script.actions.onRun
+        assert on_run.timeout == 30
+        assert on_run.cancelation is not None
+        assert on_run.cancelation.notifyPeriodInSeconds == 15
+
 
 class TestAmountRequirementEdgeCases:
     """Edge case tests for AmountRequirementTemplate."""
diff --git a/test/openjd/model_v0/v2023_09/test_wrap_actions.py b/test/openjd/model_v0/v2023_09/test_wrap_actions.py
@@ -295,6 +295,57 @@ def test_create_job_with_step_env_wrap_succeeds(self):
         step_env_actions = job.steps[0].stepEnvironments[0].script.actions
         assert step_env_actions.onWrapTaskRun is not None
 
+    def test_create_job_with_full_forwarding_defers_runtime_fields(self):
+        # Regression: a wrap hook forwarding the wrapped action's timeout and
+        # cancelation ("{{WrappedAction.Timeout}}",
+        # "{{WrappedAction.Cancelation.*}}") decoded cleanly but crashed
+        # create_job with "Undefined variable" — those symbols only exist at
+        # run time. Like openjd-rs (job/create_job/instantiate.rs clones
+        # timeout+cancelation unresolved into the Job), the fields must be
+        # carried through job instantiation as raw FormatStrings and left for
+        # the session to resolve.
+        from openjd.model._format_strings import FormatString
+
+        hook = {
+            "command": "{{WrappedAction.Command}}",
+            "args": ["{{WrappedAction.Args}}"],
+            "timeout": "{{WrappedAction.Timeout}}",
+            "cancelation": {
+                "mode": "{{WrappedAction.Cancelation.Mode}}",
+                "notifyPeriodInSeconds": ("{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}"),
+            },
+        }
+        wrap_env = {
+            "name": "W",
+            "script": {
+                "actions": {
+                    "onWrapEnvEnter": dict(hook),
+                    "onWrapTaskRun": dict(hook),
+                    "onWrapEnvExit": dict(hook),
+                }
+            },
+        }
+        exts = ("WRAP_ACTIONS", "EXPR", "FEATURE_BUNDLE_1")
+        jt = _decode_job(_job_template(job_environments=[wrap_env], extensions=exts))
+
+        # WHEN
+        job = create_job(job_template=jt, job_parameter_values={})
+
+        # THEN: creation succeeds and every run-time-resolvable field on the
+        # instantiated hook action is the raw, unresolved FormatString.
+        action = job.jobEnvironments[0].script.actions.onWrapTaskRun
+        assert isinstance(action.timeout, FormatString)
+        assert str(action.timeout) == "{{WrappedAction.Timeout}}"
+        assert isinstance(action.cancelation.mode, FormatString)
+        assert str(action.cancelation.mode) == "{{WrappedAction.Cancelation.Mode}}"
+        assert isinstance(action.cancelation.notifyPeriodInSeconds, FormatString)
+        assert (
+            str(action.cancelation.notifyPeriodInSeconds)
+            == "{{WrappedAction.Cancelation.NotifyPeriodInSeconds}}"
+        )
+        assert isinstance(action.command, FormatString)
+        assert str(action.command) == "{{WrappedAction.Command}}"
+
 
 class TestCancelationRoundTripForwarding:
     """Cancelation round-trip forwarding (openjd-rs PR #261 review,
@@ -314,10 +365,11 @@ class TestCancelationRoundTripForwarding:
     Mirrors ``cancelation_round_trip_*`` in openjd-rs
     ``crates/openjd-model/tests/integration/test_wrap_actions.rs``.
 
-    Note: the review example also forwards ``timeout:``; that is
-    deliberately out of scope pending the WrappedAction.Timeout
-    int-vs-int? question (the 0-when-unset sentinel is not a valid
-    ``<posinteger>``).
+    Note: the review example also forwards ``timeout:``; that works too —
+    see TestWrapActionsCreateJob
+    .test_create_job_with_full_forwarding_defers_runtime_fields, which
+    covers full forwarding (command/args/timeout/cancelation) through
+    create_job.
     """
 
     _ROUND_TRIP_EXTS = ["WRAP_ACTIONS", "EXPR", "FEATURE_BUNDLE_1"]
