From d42322ac728a9748d163b43e9256a51a9b88fa72 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Sat, 1 Aug 2026 15:33:27 -0400 Subject: [PATCH] Stop unresolved templates before they reach a tool (#153) Measured before this change, on a pipeline whose `content` referenced a step that does not exist: $ orchestrator run bad_template.yaml "success": true, "error": null $ cat out/bad.txt value = {{ nonexistent_step.result.field }} The literal Jinja source on disk, in a file a user would read as output, from a run that exited reporting no error. `validate` refused the same document -- so this is the mirror image of #241, with the validator right and the executor wrong. Root cause: TemplateManager catches UndefinedError, logs a warning and returns the original template text. That fallback is deliberate and has to stay, because resolution runs in several passes and a reference that cannot resolve yet may resolve later. What was missing is a point where the fallback stops being acceptable. That point is resolve_before_tool_execution, which already promised "tool parameters with all templates resolved" and is the last place the promise can be kept -- past it the value is written to a file or sent to a model. It is also the single funnel: Tool.execute routes every tool through it. The check now enforces what the docstring claimed. The test is *survival*, not presence. A marker in the output that was not in the input is content: Jinja's own escape `{{ '{{' }}` renders to a literal `{{`, and a step result may legitimately contain text that looks like a template. A first version compared only the output and rejected escaped Jinja -- caught by its own test, which is why the rule is now "appeared in the input and came back byte-identical". Nested containers are walked, since a parameter is routinely a list of paths or a dict of fields and a reference buried in one reaches a tool just as readily. `_`-prefixed parameters are skipped: those are plumbing the control systems attach on the way to a tool, not authored values. Rendering turns out to be all-or-nothing per string -- StrictUndefined aborts the whole render -- so every marker in a failed string is reported, not only the undefined one. Reporting just the bad reference would imply the others had been substituted. ADR 0001 now states the contract, alongside the action registry from #241, since neither was written down. Blocking suite 435 -> 456 passed, 13 skipped, 0 failed, 0 xfailed. Collection 3230 -> 3251, 0 warnings. lock, ruff on src, and `git diff --check` clean. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adr/0001-product-contract.md | 26 ++ src/orchestrator/core/exceptions.py | 30 +++ .../core/unified_template_resolver.py | 77 +++++- tests/test_unresolved_templates.py | 244 ++++++++++++++++++ 4 files changed, 375 insertions(+), 2 deletions(-) create mode 100644 tests/test_unresolved_templates.py diff --git a/docs/adr/0001-product-contract.md b/docs/adr/0001-product-contract.md index 4ef2c8a8..cfd8cf9e 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 3ce8e6ec..c271be94 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 254b4206..9b93e2b6 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 00000000..8e004b20 --- /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 }}"]