Stop validating fields the runtime copies verbatim (#104)

src/orchestrator/core/step_fields.py

@@ -75,3 +75,20 @@
     "dependencies",  # already read, with the `declared` origin
     "depends_on",
 })
+
+#: The same distinction one level up. A pipeline's own `name` and
+#: `description` are prose about the pipeline; nothing renders them either, so
+#: `name: "{{ nosuch }}"` at the top of a document was failing validation for a
+#: pipeline that runs.
+#:
+#: Deliberately short. `outputs` *is* rendered; `parameters` declares names
+#: rather than using them; and `id` and `version` are schema-constrained --
+#: `version` must match `\d+\.\d+\.\d+`, so a template there is a real error
+#: and calling the field inert would describe it wrongly. Only fields a real
+#: run tolerates an unresolvable reference in are listed, which is what
+#: `test_a_pipeline_with_templates_in_prose_still_runs` checks.
+INERT_PIPELINE_FIELDS: FrozenSet[str] = frozenset({
+    "name",
+    "description",
+    "metadata",
+})

src/orchestrator/validation/template_validator.py

@@ -19,6 +19,7 @@
 
 from ..core.runtime_context import BARE_RUNTIME_NAMES, RUNTIME_NAMESPACE
 from ..core.loop_contracts import ALL_BINDINGS, LoopContract, contracts_for
+from ..core.step_fields import INERT_PIPELINE_FIELDS, INERT_STEP_FIELDS
 from ..core.template_globals import (
     ALL_LOOP_VARIABLES,
     DOLLAR_LOOP_VARIABLES,
@@ -39,6 +40,17 @@
 #: this is where a loop construct is looked for -- see `_validate_object_templates`.
 _STEP_PATH = re.compile(r"steps\[\d+\]$")
 
+#: What a template in an inert field actually does. Validation used to report
+#: `{{ b.result }}` in a step's `name:` as "references step results - will be
+#: resolved at runtime", which is the opposite of true: nothing renders `name`,
+#: so the braces reach the log verbatim. Worse, `{{ nosuch }}` in a
+#: `description:` was a hard error, so a stray brace in prose rejected a
+#: pipeline that runs correctly.
+_INERT_TEMPLATE_MESSAGE = (
+    "'{field}' is copied verbatim, so this template is never rendered -- "
+    "the braces appear literally in the output"
+)
+
 
 def _binding_set(value: Union[bool, FrozenSet[str], None]) -> FrozenSet[str]:
     """Normalise the loop-scope argument to a set of names.
@@ -605,6 +617,7 @@ def _validate_object_templates(
         undefined_variables: Set,
         loop_bindings: FrozenSet[str] = frozenset(),
         loop_scope: Optional[Tuple[LoopContract, str, FrozenSet[str]]] = None,
+        inert_field: Optional[str] = None,
     ):
         """Recursively validate templates in an object.
 
@@ -613,10 +626,29 @@ def _validate_object_templates(
         path reached inside it, because scope is a property of the field and
         not of the step: a `create_parallel_queue`'s `on` resolves before any
         item exists while the action list beside it runs per item.
+
+        `inert_field` names the step field the runtime copies verbatim, if
+        this walk is inside one. Nothing substitutes into it, so a reference
+        there cannot be undefined and cannot be resolved later. The *field* is
+        carried rather than a flag so a nested value reports `metadata` -- the
+        field that is inert -- instead of whichever key it sits under.
         """
         if isinstance(obj, str):
             # Check if this contains templates
             if '{{' in obj or '{%' in obj:
+                if inert_field:
+                    warnings.append(TemplateValidationError(
+                        template=obj,
+                        error_type="inert_field_template",
+                        message=_INERT_TEMPLATE_MESSAGE.format(field=inert_field),
+                        context_path=path,
+                        severity="warning",
+                        suggestions=[
+                            "Move the reference to a field that is rendered "
+                            "(parameters, action, location), or remove the braces"
+                        ],
+                    ))
+                    return
                 result = self.validate_template(
                     obj, context, path, step_ids, loop_bindings
                 )
@@ -631,7 +663,8 @@ def _validate_object_templates(
             # `action_loop` look like a second, separate loop, which replaced
             # the queue's scope with the action loop's and let `{{ item }}`
             # through in the `on` expression that generates the queue.
-            declared = contracts_for(obj) if _STEP_PATH.search(path) else ()
+            is_step = bool(_STEP_PATH.search(path))
+            declared = contracts_for(obj) if is_step else ()
             if len(declared) > 1:
                 # Which construct wins is decided by declaration order in
                 # `loop_contracts`, and no engine agreed to that order. The
@@ -659,6 +692,11 @@ def _validate_object_templates(
 
             for key, value in obj.items():
                 new_path = f"{path}.{key}" if path else key
+                inert_here = (
+                    (is_step and key in INERT_STEP_FIELDS)
+                    or (not path and key in INERT_PIPELINE_FIELDS)
+                )
+                child_inert = inert_field or (key if inert_here else None)
                 child_bindings, child_scope = loop_bindings, loop_scope
                 if loop_scope is not None:
                     contract, prefix, enclosing = loop_scope
@@ -668,7 +706,7 @@ def _validate_object_templates(
                 self._validate_object_templates(
                     value, context, step_ids, new_path,
                     errors, warnings, used_variables, undefined_variables,
-                    child_bindings, child_scope,
+                    child_bindings, child_scope, child_inert,
                 )
 
         elif isinstance(obj, list):
@@ -677,7 +715,7 @@ def _validate_object_templates(
                 self._validate_object_templates(
                     item, context, step_ids, new_path,
                     errors, warnings, used_variables, undefined_variables,
-                    loop_bindings, loop_scope,
+                    loop_bindings, loop_scope, inert_field,
                 )
 
     def _is_step_result_reference(self, var_name: str, step_ids: List[str]) -> bool:

tests/test_inert_field_diagnostics.py

@@ -0,0 +1,152 @@
+"""A template in a field nothing renders is not a runtime promise.
+
+`core.step_fields` established which parts of a step the runtime renders, and
+#471 wired it into dependency inference so two inert strings stopped inventing
+a cycle. Template validation was still reading those same fields as if they
+resolved, and got both directions wrong:
+
+    - id: a
+      name: "{{ b.result }}"          # "will be resolved at runtime" -- it is not
+      description: "{{ nosuch }}"     # a hard error, in prose nobody renders
+
+The first is a false promise: nothing substitutes into `name`, so the braces
+reach the log verbatim and the reader is told the opposite. The second is a
+false rejection: a stray brace in a description failed a pipeline that runs
+correctly, which is the class of bug #465, #469 and #472 each removed
+elsewhere.
+
+The run in `test_a_pipeline_with_templates_in_prose_still_runs` is the check
+that this is a real property of the runtime and not a claim about it.
+"""
+
+import asyncio
+from pathlib import Path
+
+import pytest
+
+from orchestrator.core.step_fields import (
+    INERT_STEP_FIELDS,
+    RENDERABLE_STEP_FIELDS,
+)
+from orchestrator.validation.template_validator import TemplateValidator
+from tests.test_infrastructure import create_test_orchestrator
+
+pytestmark = [pytest.mark.contract]
+
+#: `id` names the step and `dependencies`/`depends_on` hold step ids. They are
+#: inert for the same reason, but a template in one is a structural error
+#: rather than prose, so they are not probed as free text here.
+PROSE_FIELDS = sorted(INERT_STEP_FIELDS - {"id", "dependencies", "depends_on", "tool"})
+
+
+def _validate(step, context=None):
+    return TemplateValidator().validate_pipeline_templates(
+        {"id": "p", "steps": [step, {"id": "b", "parameters": {}}]},
+        context or {},
+    )
+
+
+@pytest.mark.parametrize("field", PROSE_FIELDS)
+def test_an_undefined_name_in_an_inert_field_is_not_an_error(field):
+    """It cannot be undefined: it is never looked up."""
+    result = _validate({"id": "a", field: "{{ nosuch_variable }}"})
+    assert result.is_valid, [
+        (e.error_type, e.context_path, e.message) for e in result.errors
+    ]
+
+
+@pytest.mark.parametrize("field", PROSE_FIELDS)
+def test_an_inert_field_still_warns(field):
+    """Silence would be wrong too -- the author wrote a template and will get
+    braces. The warning is how they find out before reading the output."""
+    result = _validate({"id": "a", field: "{{ b.result }}"})
+    kinds = [w.error_type for w in result.warnings]
+    assert "inert_field_template" in kinds, kinds
+
+
+def test_the_warning_does_not_claim_the_value_arrives_later():
+    """The old message said `will be resolved at runtime`, which is the one
+    thing that does not happen."""
+    result = _validate({"id": "a", "name": "{{ b.result }}"})
+    inert = [w for w in result.warnings if w.error_type == "inert_field_template"]
+    assert inert, [w.error_type for w in result.warnings]
+    assert "resolved at runtime" not in inert[0].message
+    assert "never rendered" in inert[0].message
+
+
+def test_the_warning_names_the_inert_field_not_the_key_beneath_it():
+    """`metadata.note` is inert because `metadata` is. Naming `note` would
+    send the reader looking for a rule about a key they invented."""
+    result = _validate({"id": "a", "metadata": {"note": "{{ b.result }}"}})
+    inert = [w for w in result.warnings if w.error_type == "inert_field_template"]
+    assert inert and "'metadata'" in inert[0].message, [w.message for w in inert]
+    assert inert[0].context_path == "steps[0].metadata.note", inert[0].context_path
+
+
+@pytest.mark.parametrize("field", RENDERABLE_STEP_FIELDS)
+def test_a_renderable_field_still_reports_an_undefined_name(field):
+    """The suppression must not spread. These fields do resolve, so a name
+    that is not there is still an error."""
+    value = {"x": "{{ nosuch_variable }}"} if field == "parameters" else "{{ nosuch_variable }}"
+    result = _validate({"id": "a", field: value})
+    assert not result.is_valid, f"{field} is rendered; an undefined name there is an error"
+    assert "undefined_variable" in [e.error_type for e in result.errors]
+
+
+def test_a_step_result_reference_in_a_rendered_field_is_still_a_runtime_promise():
+    result = _validate({"id": "a", "parameters": {"x": "{{ b.result }}"}})
+    assert "runtime_variable" in [w.error_type for w in result.warnings]
+
+
+@pytest.mark.e2e
+def test_a_pipeline_with_templates_in_prose_still_runs(tmp_path):
+    """The evidence that these fields are inert, rather than the assertion.
+
+    Every prose field carries a reference to a name that exists nowhere. If
+    any of them were rendered the run would fail or write the wrong thing; the
+    file that lands proves it did neither.
+    """
+    pipeline = f"""
+id: inert_prose
+name: "{{{{ nosuch_pipeline_name }}}}"
+description: "{{{{ nosuch_description }}}}"
+metadata:
+  owner: "{{{{ nosuch_owner }}}}"
+steps:
+  - id: write_it
+    name: "{{{{ nosuch_step_name }}}}"
+    description: "{{{{ nosuch_step_description }}}}"
+    metadata:
+      note: "{{{{ nosuch_metadata }}}}"
+    tool: filesystem
+    action: write
+    parameters:
+      path: "{tmp_path}/out.txt"
+      content: "written"
+"""
+    asyncio.run(create_test_orchestrator().execute_yaml(pipeline, {}))
+    written = Path(tmp_path, "out.txt")
+    assert written.exists(), "the step did not run"
+    assert written.read_text() == "written"
+
+
+@pytest.mark.e2e
+def test_that_same_pipeline_validates(tmp_path):
+    """Both surfaces agree: what runs, validates."""
+    result = TemplateValidator().validate_pipeline_templates(
+        {
+            "id": "inert_prose",
+            "name": "{{ nosuch_pipeline_name }}",
+            "steps": [{
+                "id": "write_it",
+                "name": "{{ nosuch_step_name }}",
+                "description": "{{ nosuch_step_description }}",
+                "metadata": {"note": "{{ nosuch_metadata }}"},
+                "tool": "filesystem",
+                "action": "write",
+                "parameters": {"path": str(tmp_path / "out.txt"), "content": "written"},
+            }],
+        },
+        {},
+    )
+    assert result.is_valid, [(e.error_type, e.context_path) for e in result.errors]