diff --git a/src/orchestrator/core/step_fields.py b/src/orchestrator/core/step_fields.py index 76cd01f..61df6c9 100644 --- a/src/orchestrator/core/step_fields.py +++ b/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", +}) diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index 791b381..e7f29ce 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/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: diff --git a/tests/test_inert_field_diagnostics.py b/tests/test_inert_field_diagnostics.py new file mode 100644 index 0000000..11e6963 --- /dev/null +++ b/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]