diff --git a/docs/adr/0001-product-contract.md b/docs/adr/0001-product-contract.md index 4ef2c8a..cfd8cf9 100644 --- a/docs/adr/0001-product-contract.md +++ b/docs/adr/0001-product-contract.md @@ -114,6 +114,32 @@ missing extra must degrade one feature, never break the package import. Exit codes: `0` success, `1` execution failure, `2` validation/compile failure, `130` interrupted. +## Actions and template resolution + +A step names either a tool and an operation on it (`tool: filesystem` with +`action: read`), or an action the runtime executes itself (`action: generate`). +`core/actions.py` is the single source of truth for the second group: +`HybridControlSystem` dispatches off it and `ToolValidator` accepts exactly the +names in it, so `validate` and `run` cannot disagree about whether an action +exists (#241). + +Template rendering deliberately falls back to returning the original text when +a reference is undefined, because resolution runs in several passes and a +reference that cannot resolve yet may resolve later. That fallback must not +survive the handoff to a tool: + +> A template reference that reaches a tool still unresolved fails the step +> before any side effect. No file is written, the step's envelope carries +> `success: false` naming the reference, and the run exits 1. + +The test is *survival*, not presence: a marker in the rendered output that was +not in the input is content, not a failure. Jinja's own escape `{{ '{{' }}` +renders to a literal `{{`, and a step result may legitimately contain text that +looks like a template (#153). + +Rendering is all-or-nothing per string. One undefined reference aborts the +whole render, so every marker in that string is reported, not just the bad one. + ## Test layers | Layer | Marker | Network | Secrets | Runs in default CI | diff --git a/src/orchestrator/core/exceptions.py b/src/orchestrator/core/exceptions.py index 3ce8e6e..c271be9 100644 --- a/src/orchestrator/core/exceptions.py +++ b/src/orchestrator/core/exceptions.py @@ -205,6 +205,36 @@ def __init__(self, parameter: str, reason: str, **kwargs): ) +class UnresolvedTemplateError(ValidationError): + """Raised when a template reaches a tool with references still unresolved. + + Template rendering deliberately falls back to returning the original text + when a reference is undefined, because resolution runs in several passes + and a reference that cannot be resolved yet may resolve later. The one + place that fallback must not survive is the handoff to a tool: past that + point the literal `{{ ... }}` is written to a file, sent to a model, or + returned as a result, and the step reports success having produced its own + source code instead of a value (#153). + """ + + def __init__(self, tool_name: str, parameter: str, unresolved: list, **kwargs): + rendered = ", ".join(repr(marker) for marker in unresolved) + message = ( + f"Parameter '{parameter}' of tool '{tool_name}' still contains " + f"unresolved template references: {rendered}. The step was stopped " + f"before it could act on them." + ) + super().__init__( + message, + details={ + "tool": tool_name, + "parameter": parameter, + "unresolved": list(unresolved), + }, + **kwargs + ) + + # Resource errors class ResourceError(OrchestratorError): """Base class for resource-related errors.""" diff --git a/src/orchestrator/core/unified_template_resolver.py b/src/orchestrator/core/unified_template_resolver.py index 254b420..9b93e2b 100644 --- a/src/orchestrator/core/unified_template_resolver.py +++ b/src/orchestrator/core/unified_template_resolver.py @@ -10,16 +10,46 @@ """ import logging +import re from typing import Any, Dict, List, Optional, Union from dataclasses import dataclass from contextlib import contextmanager +from .exceptions import UnresolvedTemplateError from .template_manager import TemplateManager from .context_manager import ContextManager from .loop_context import LoopContextVariables, GlobalLoopContextManager logger = logging.getLogger(__name__) +# `{{ ... }}` substitutions and `{% ... %}` control blocks. Comments (`{# #}`) +# are not included: they are erased by rendering, so one surviving is a syntax +# problem the renderer already reports. +_TEMPLATE_MARKER = re.compile(r"\{\{.*?\}\}|\{%.*?%\}", re.DOTALL) + + +def _find_template_markers(value: Any) -> List[str]: + """Every template marker surviving anywhere inside `value`. + + Walks nested containers, because a parameter is routinely a list of paths + or a dict of fields and an unresolved reference buried in one of those + reaches a tool exactly as readily as a top-level string does. + """ + if isinstance(value, str): + return _TEMPLATE_MARKER.findall(value) + if isinstance(value, dict): + found = [] + for key, item in value.items(): + found.extend(_find_template_markers(key)) + found.extend(_find_template_markers(item)) + return found + if isinstance(value, (list, tuple, set)): + found = [] + for item in value: + found.extend(_find_template_markers(item)) + return found + return [] + @dataclass class TemplateResolutionContext: @@ -334,14 +364,57 @@ def resolve_before_tool_execution( # Register context and resolve templates self.register_context(context) resolved_parameters = self.resolve_templates(tool_parameters) - + if self.debug_mode: logger.info(f"Resolved templates for tool '{tool_name}'") for key, (original, resolved) in self._compare_dicts(tool_parameters, resolved_parameters).items(): if original != resolved: logger.debug(f" {key}: '{original}' -> '{resolved}'") - + + # This method promises "all templates resolved", and it is the last + # point at which that promise can still be kept: past here the value is + # written to a file or sent to a model. Rendering falls back to + # returning the original text when a reference is undefined, so without + # this check a literal `{{ step.field }}` lands in the output and the + # step reports success (#153). + self._reject_unresolved_templates( + tool_name, tool_parameters, resolved_parameters + ) + return resolved_parameters + + def _reject_unresolved_templates( + self, + tool_name: str, + original: Dict[str, Any], + resolved: Dict[str, Any], + ) -> None: + """Raise if a template reference survived rendering unchanged. + + The test is *survival*, not mere presence. A marker in the output that + was not in the input is content, not a failure to resolve: Jinja's own + escape, `{{ '{{' }}`, renders to a literal `{{`, and a step result may + legitimately contain text that looks like a template. Flagging those + would make it impossible to write documentation about templates. + + A marker that appears in the input and comes back byte-identical did + not render, which is exactly the #153 defect. + + Parameters whose names begin with an underscore are internal plumbing + the control systems attach on the way to a tool (`_template_manager`, + `_resolution_context` and friends), not user-authored values, so they + are not inspected. + """ + for name, value in resolved.items(): + if name.startswith("_"): + continue + after = _find_template_markers(value) + if not after: + continue + before = set(_find_template_markers(original.get(name))) + survived = [marker for marker in after if marker in before] + if survived: + raise UnresolvedTemplateError(tool_name, name, survived) def validate_templates(self, template: str) -> List[str]: """Validate template syntax and check for undefined variables. diff --git a/tests/test_unresolved_templates.py b/tests/test_unresolved_templates.py new file mode 100644 index 0000000..8e004b2 --- /dev/null +++ b/tests/test_unresolved_templates.py @@ -0,0 +1,244 @@ +"""A template reference that never resolved must not reach a tool. + +Template rendering deliberately falls back to returning the original text when +a reference is undefined, because resolution runs in several passes and a +reference that cannot resolve yet may resolve later. The failure mode (#153) is +that the fallback survived all the way to the tool: the literal +`{{ step.field }}` was written to a file and the step reported `success: true`. + +Measured before the fix, on the pipeline below: + + $ orchestrator run bad_template.yaml + "success": true, "error": null + $ cat out/bad.txt + value = {{ nonexistent_step.result.field }} + +The complementary case -- a template that *does* resolve must not be written +through literally -- is +tests/test_golden_pipelines.py::test_unresolved_template_does_not_reach_output. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from orchestrator.core.exceptions import UnresolvedTemplateError +from orchestrator.core.unified_template_resolver import ( + UnifiedTemplateResolver, + _find_template_markers, +) + +pytestmark = [pytest.mark.contract, pytest.mark.e2e] + +# Exit codes, from docs/adr/0001-product-contract.md. +EXIT_EXECUTION_ERROR = 1 + +UNRESOLVED_PIPELINE = """ +id: unresolved_template_pipeline +name: Unresolved Template Pipeline +steps: + - id: write_it + tool: filesystem + action: write + parameters: + path: "out/bad.txt" + content: "value = {{ nonexistent_step.result.field }}" +""" + + +def _run_cli(args, cwd): + """Invoke the CLI as a subprocess, the way a user would.""" + env = dict(os.environ) + src = str(Path(__file__).parent.parent / "src") + env["PYTHONPATH"] = src + os.pathsep + env.get("PYTHONPATH", "") + env.pop("ANTHROPIC_API_KEY", None) + env["ORCHESTRATOR_AUTO_INSTALL"] = "0" + return subprocess.run( + [sys.executable, "-m", "orchestrator.cli", *args], + cwd=str(cwd), + env=env, + capture_output=True, + text=True, + timeout=300, + ) + + +# --------------------------------------------------------------------------- +# end to end +# --------------------------------------------------------------------------- + +def test_an_unresolved_reference_fails_the_run_and_writes_nothing(tmp_path): + """The whole point: no artifact, non-zero exit, and the reason is named.""" + pipeline = tmp_path / "bad_template.yaml" + pipeline.write_text(UNRESOLVED_PIPELINE) + + result = _run_cli(["run", str(pipeline)], cwd=tmp_path) + + assert result.returncode == EXIT_EXECUTION_ERROR, ( + f"expected exit {EXIT_EXECUTION_ERROR}, got {result.returncode}\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + assert not (tmp_path / "out" / "bad.txt").exists(), ( + "the step was stopped, so it must not have written its output file" + ) + + combined = result.stdout + result.stderr + assert "nonexistent_step" in combined, ( + f"the error must name the reference that failed:\n{combined[-2000:]}" + ) + + +# --------------------------------------------------------------------------- +# the check itself +# --------------------------------------------------------------------------- + +@pytest.fixture +def resolver(): + return UnifiedTemplateResolver() + + +def _resolve(resolver, parameters): + context = resolver.collect_context(pipeline_id="p", task_id="t") + return resolver.resolve_before_tool_execution("filesystem", parameters, context) + + +def test_a_resolvable_template_still_passes(resolver): + """No false positives: a reference that resolves must go straight through.""" + context = resolver.collect_context( + pipeline_id="p", task_id="t", pipeline_inputs={"name": "world"} + ) + resolved = resolver.resolve_before_tool_execution( + "filesystem", {"content": "hello {{ name }}"}, context + ) + + assert resolved["content"] == "hello world" + + +def test_an_unresolved_reference_raises_naming_the_parameter(resolver): + with pytest.raises(UnresolvedTemplateError) as excinfo: + _resolve(resolver, {"content": "value = {{ missing.field }}"}) + + error = excinfo.value + assert error.details["parameter"] == "content" + assert error.details["tool"] == "filesystem" + assert error.details["unresolved"] == ["{{ missing.field }}"] + # The message has to be usable without opening a debugger. + assert "content" in str(error) and "missing.field" in str(error) + + +def test_a_surviving_control_block_is_caught_too(resolver): + """`{% if %}` reaching a tool is the same defect wearing different syntax.""" + with pytest.raises(UnresolvedTemplateError): + _resolve(resolver, {"content": "{% if missing %}yes{% endif %}"}) + + +def test_a_reference_buried_in_a_container_is_caught(resolver): + """Parameters are routinely lists of paths or dicts of fields. + + An unresolved reference nested inside one reaches the tool exactly as + readily as a top-level string, so scanning only top-level strings would + leave the hole open. + """ + with pytest.raises(UnresolvedTemplateError) as excinfo: + _resolve(resolver, {"paths": ["ok.txt", {"nested": "{{ missing }}"}]}) + + assert excinfo.value.details["parameter"] == "paths" + + +def test_internal_plumbing_parameters_are_not_inspected(resolver): + """The control systems attach `_`-prefixed objects on the way to a tool. + + Those are machinery, not user-authored values, and one of them holding a + template-looking string must not fail the step. + """ + resolved = _resolve( + resolver, {"content": "fine", "_internal_note": "{{ not_user_authored }}"} + ) + + assert resolved["_internal_note"] == "{{ not_user_authored }}" + + +def test_escaped_delimiters_are_not_a_false_positive(resolver): + """A pipeline may legitimately emit literal Jinja, e.g. documentation. + + Jinja's own escape renders to the delimiters as text, and by then there is + no marker left to find -- so this must pass, or writing a tutorial about + templates would be impossible. + """ + resolved = _resolve(resolver, {"content": "write {{ '{{' }} name {{ '}}' }} here"}) + + assert resolved["content"] == "write {{ name }} here" + + +def test_a_resolved_value_containing_template_text_is_content_not_a_failure(resolver): + """The test is survival, not presence. + + A step whose *result* contains something shaped like a template -- a + scraped page, a code sample, a prompt about Jinja -- has resolved + perfectly well. Flagging it would mean the pipeline can never carry text + about templates through a tool. + """ + context = resolver.collect_context( + pipeline_id="p", + task_id="t", + pipeline_inputs={"snippet": "use {{ user.name }} to interpolate"}, + ) + resolved = resolver.resolve_before_tool_execution( + "filesystem", {"content": "{{ snippet }}"}, context + ) + + assert resolved["content"] == "use {{ user.name }} to interpolate" + + +def test_one_bad_reference_takes_the_whole_string_down(resolver): + """Rendering is all-or-nothing per string, and the report says so. + + `StrictUndefined` raises on the first undefined reference, and the fallback + returns the *entire* original string -- so in `"{{ here }} but not + {{ missing }}"` the resolvable `{{ here }}` does not survive either. Both + markers are reported, which is the truthful account: nothing in this string + was rendered. Reporting only `{{ missing }}` would imply `{{ here }}` had + been substituted, and it had not. + """ + context = resolver.collect_context( + pipeline_id="p", task_id="t", pipeline_inputs={"here": "ok"} + ) + + with pytest.raises(UnresolvedTemplateError) as excinfo: + resolver.resolve_before_tool_execution( + "filesystem", {"content": "{{ here }} but not {{ missing }}"}, context + ) + + assert excinfo.value.details["unresolved"] == ["{{ here }}", "{{ missing }}"] + + +# --------------------------------------------------------------------------- +# the scanner +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "value,expected", + [ + ("plain text", []), + ("{{ a }}", ["{{ a }}"]), + ("{{ a }} and {{ b }}", ["{{ a }}", "{{ b }}"]), + ("{% for x in y %}", ["{% for x in y %}"]), + ({"k": "{{ a }}"}, ["{{ a }}"]), + ({"{{ key }}": "v"}, ["{{ key }}"]), + ([["{{ deep }}"]], ["{{ deep }}"]), + (("{{ t }}",), ["{{ t }}"]), + (42, []), + (None, []), + ("", []), + ], +) +def test_find_template_markers(value, expected): + assert _find_template_markers(value) == expected + + +def test_find_template_markers_spans_newlines(): + """A multi-line block is still one marker, not two halves of nothing.""" + assert _find_template_markers("{{ a\n + b }}") == ["{{ a\n + b }}"]