Make the shared template registry actually shared (#104)

src/orchestrator/compiler/yaml_compiler.py

@@ -151,9 +151,6 @@ def __init__(
 
         self.template_engine = create_pipeline_environment()
 
-        # Add custom filters to Jinja2 environment
-        self._register_custom_filters()
-
         # Regex pattern for AUTO tags (with optional attributes)
         self.auto_tag_pattern = re.compile(r"<AUTO[^>]*>(.*?)</AUTO>", re.DOTALL)
 
@@ -1549,116 +1546,6 @@ def validate_yaml(self, yaml_content: str) -> bool:
         except Exception:
             return False
 
-    def _register_custom_filters(self) -> None:
-        """Register custom Jinja2 filters."""
-        import re as regex_module
-
-        def regex_search(value, pattern, group=None):
-            """Search for regex pattern in value."""
-            if not isinstance(value, str):
-                value = str(value)
-            match = regex_module.search(pattern, value, regex_module.DOTALL)
-            if match:
-                if group is not None:
-                    try:
-                        # Handle numeric group references
-                        if isinstance(group, str) and group.startswith("\\"):
-                            group_num = int(group[1:])
-                            return (
-                                match.group(group_num)
-                                if group_num <= match.lastindex
-                                else ""
-                            )
-                        return match.group(group)
-                    except (IndexError, ValueError):
-                        return ""
-                return match.group(0)
-            return ""
-
-        # Register filters
-        self.template_engine.filters["regex_search"] = regex_search
-
-        # Also add other commonly used filters that might be missing
-        self.template_engine.filters["default"] = lambda v, d="": v if v else d
-        self.template_engine.filters["lower"] = lambda v: str(v).lower()
-        self.template_engine.filters["upper"] = lambda v: str(v).upper()
-        self.template_engine.filters["replace"] = lambda v, old, new: str(v).replace(
-            old, new
-        )
-
-        # Add missing filters
-        import json
-        from datetime import datetime
-        import re as re_module
-
-        # Slugify filter
-        def slugify(value):
-            """Convert string to slug format."""
-            value = str(value).lower()
-            # Replace spaces and underscores with hyphens
-            value = re_module.sub(r"[\s_]+", "-", value)
-            # Remove non-alphanumeric characters except hyphens
-            value = re_module.sub(r"[^a-z0-9-]", "", value)
-            # Remove multiple consecutive hyphens
-            value = re_module.sub(r"-+", "-", value)
-            # Strip hyphens from start and end
-            return value.strip("-")
-
-        # Date filter
-        def date_filter(value, format="%Y-%m-%d"):
-            """Format datetime value."""
-            if isinstance(value, str):
-                # Parse ISO format
-                try:
-                    value = datetime.fromisoformat(value.replace("Z", "+00:00"))
-                except Exception:
-                    value = datetime.now()
-            elif not isinstance(value, datetime):
-                value = datetime.now()
-
-            # Handle special format strings
-            format = format.replace("Y", "%Y").replace("m", "%m").replace("d", "%d")
-            format = format.replace("H", "%H").replace("i", "%M").replace("s", "%S")
-            return value.strftime(format)
-
-        # JSON filter
-        def json_filter(value, indent=None):
-            """Convert value to JSON string."""
-            return json.dumps(value, indent=indent, default=str)
-
-        # Now function for templates
-        def now():
-            """Return current datetime."""
-            return datetime.now()
-
-        # from_json filter
-        def from_json(value):
-            """Parse JSON string to object."""
-            if isinstance(value, str):
-                try:
-                    return json.loads(value)
-                except Exception:
-                    return value
-            return value
-
-        # Basename filter
-        def basename(value):
-            """Get the basename of a path."""
-            import os
-            return os.path.basename(str(value))
-
-        self.template_engine.filters["slugify"] = slugify
-        self.template_engine.filters["date"] = date_filter
-        self.template_engine.filters["json"] = json_filter
-        self.template_engine.filters["from_json"] = from_json
-        self.template_engine.filters["to_json"] = json_filter  # Alias for json
-        self.template_engine.filters["basename"] = basename
-        self.template_engine.globals["now"] = now
-
-        # Add special variables that should not be processed as regular templates
-        # These will be handled by the control flow system
-        self.special_vars = {"$item", "$index", "$is_first", "$is_last"}
-
     def get_template_variables(self, yaml_content: str) -> List[str]:
         """
         Extract template variables from YAML content.

tests/test_template_language_conformance.py

@@ -0,0 +1,244 @@
+"""One pipeline language, rendered identically wherever it is rendered.
+
+A pipeline's `{{ }}` expressions pass through several Jinja environments: the
+template validator checks them, the YAML compiler renders the ones it can
+resolve at compile time, and the runtime renders the rest. If those
+environments disagree, an expression means different things depending on which
+one reaches it -- and the disagreement shows up as a pipeline that validates
+and then fails, or fails to compile and then runs correctly.
+
+#448 made all three environments share one filter registry, and asserted it by
+comparing *filter names*. Matching names are not matching behaviour. The
+compiler re-registered eleven of those filters immediately afterwards with its
+own implementations, so the name sets agreed while the semantics did not:
+
+    {{ 0 | default('X') }}          runtime '0'   compiler 'X'
+    {{ '' | default('X') }}         runtime ''    compiler 'X'
+    {{ missing | default('X') }}    runtime 'X'   compiler UndefinedError
+
+The last one is the case `default` exists for, and the compiler was the only
+environment that could not do it.
+
+These tests compare *results*, not registries: the same expression and input
+through every environment, asserting the same value or the same failure.
+"""
+
+import os
+import re
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+from orchestrator.compiler.yaml_compiler import YAMLCompiler
+from orchestrator.core.template_manager import TemplateManager
+from orchestrator.validation.template_validator import TemplateValidator
+
+pytestmark = [pytest.mark.contract]
+
+REPO = Path(__file__).parent.parent
+
+
+def _environments():
+    """Every environment a pipeline template can be rendered by."""
+    return {
+        "runtime": TemplateManager().env,
+        "compiler": YAMLCompiler().template_engine,
+        "validator": TemplateValidator().env,
+    }
+
+
+def _outcome(env, expression, context):
+    """('value', rendered) or ('raised', ExceptionName) -- comparable either way."""
+    try:
+        return ("value", env.from_string(expression).render(**context))
+    except Exception as exc:  # noqa: BLE001 - the class is the observation
+        return ("raised", type(exc).__name__)
+
+
+#: (id, expression, context). Chosen for the places filter implementations
+#: usually diverge: the difference between undefined and falsy, None handling,
+#: non-ASCII text, and malformed input.
+CASES = [
+    # `default` must distinguish "not defined" from "defined and falsy". This
+    # is the whole point of the filter and where the compiler's copy was wrong.
+    ("default_undefined", "{{ missing | default('X') }}", {}),
+    ("default_zero", "{{ v | default('X') }}", {"v": 0}),
+    ("default_empty_string", "{{ v | default('X') }}", {"v": ""}),
+    ("default_false", "{{ v | default('X') }}", {"v": False}),
+    ("default_none", "{{ v | default('X') }}", {"v": None}),
+    ("default_empty_list", "{{ v | default('X') }}", {"v": []}),
+    ("default_present", "{{ v | default('X') }}", {"v": "real"}),
+    # `default(..., true)` is the opt-in falsy form and must stay distinct.
+    ("default_boolean_form", "{{ v | default('X', true) }}", {"v": 0}),
+    # Case filters on non-ASCII: str.lower() and str.upper() are not
+    # interchangeable across scripts.
+    ("lower_unicode", "{{ v | lower }}", {"v": "ÄÖÜ Straße ÉCOLE"}),
+    ("upper_unicode", "{{ v | upper }}", {"v": "äöü straße école"}),
+    ("lower_non_string", "{{ v | lower }}", {"v": 42}),
+    # Malformed input: whatever the answer is, it must be the same answer.
+    ("from_json_invalid", "{{ v | from_json }}", {"v": "Hello World Report"}),
+    ("from_json_valid", '{{ v | from_json }}', {"v": '{"a": 1}'}),
+    ("from_json_empty", "{{ v | from_json }}", {"v": ""}),
+    ("to_json_unicode", "{{ v | to_json }}", {"v": {"k": "café"}}),
+    # Regex: no match, special characters, and a group.
+    ("regex_no_match", "{{ v | regex_search('zzz') }}", {"v": "abc"}),
+    ("regex_match", "{{ v | regex_search('b.') }}", {"v": "abc"}),
+    ("regex_special", "{{ v | regex_search('a.c') }}", {"v": "a.c"}),
+    # Path handling.
+    ("basename_plain", "{{ '/a/b/c.txt' | basename }}", {}),
+    ("basename_trailing_slash", "{{ '/a/b/' | basename }}", {}),
+    ("basename_empty", "{{ '' | basename }}", {}),
+    # slugify on text that is not already slug-shaped.
+    ("slugify_unicode", "{{ v | slugify }}", {"v": "Café Résumé 2024!"}),
+    ("slugify_empty", "{{ v | slugify }}", {"v": ""}),
+    ("replace_basic", "{{ v | replace('a', 'b') }}", {"v": "banana"}),
+]
+
+
+@pytest.mark.parametrize(
+    "expression,context", [(e, c) for _, e, c in CASES], ids=[i for i, _, _ in CASES]
+)
+def test_every_environment_renders_the_same_result(expression, context):
+    """Same expression, same input, same answer -- or the same failure."""
+    outcomes = {
+        name: _outcome(env, expression, context)
+        for name, env in _environments().items()
+    }
+
+    distinct = set(outcomes.values())
+    assert len(distinct) == 1, (
+        f"{expression!r} with {context!r} means different things depending on "
+        f"which environment renders it: "
+        + ", ".join(f"{n}={o!r}" for n, o in outcomes.items())
+    )
+
+
+def test_the_environments_offer_the_same_filters():
+    """Names, as well as behaviour. Both are required; neither is sufficient."""
+    runtime = set(TemplateManager().env.filters)
+    drifted = {
+        name: sorted(runtime.symmetric_difference(set(env.filters)))
+        for name, env in _environments().items()
+        if set(env.filters) != runtime
+    }
+    assert not drifted, f"filter registries differ from the runtime: {drifted}"
+
+
+#: Probe inputs for the sweep below. Deliberately includes the values that
+#: separate "undefined" from "falsy", plus non-ASCII and a non-string.
+PROBES = [{"v": v} for v in ("text", "", "Café Résumé", 0, False, None, 42, ["a"])]
+
+#: Filters whose output is not a function of their input, so "the same answer
+#: everywhere" is not a property they can have. Jinja's own, not ours.
+NONDETERMINISTIC_FILTERS = frozenset({"random", "shuffle"})
+
+_ADDRESS = re.compile(r"0x[0-9a-fA-F]+")
+
+
+def _comparable(outcome):
+    """Rendered output with object addresses masked.
+
+    Lazy filters render as `<generator object do_items at 0x10b0e2190>`. The
+    address differs between any two renders, including two of the same
+    environment, so comparing it raw reports every lazy filter as divergent.
+    """
+    kind, payload = outcome
+    return (kind, _ADDRESS.sub("0xADDR", payload) if kind == "value" else payload)
+
+
+def test_no_filter_diverges_between_environments():
+    """Every shared filter, not only the ones this file thought to name.
+
+    The cases above were chosen by guessing where implementations diverge, and
+    that is exactly how the original name-only check missed the problem: it
+    tested what I remembered to test. This sweeps the whole registry, so a
+    filter re-registered with different behaviour is caught whether or not
+    anyone anticipated it.
+
+    A filter that raises for a probe is fine -- it only has to raise the same
+    way everywhere.
+    """
+    environments = _environments()
+    runtime = environments["runtime"]
+
+    divergent = {}
+    for filter_name in sorted(runtime.filters):
+        if filter_name in NONDETERMINISTIC_FILTERS:
+            continue
+        expression = "{{ v | " + filter_name + " }}"
+        for probe in PROBES:
+            outcomes = {
+                name: _comparable(_outcome(env, expression, probe))
+                for name, env in environments.items()
+            }
+            if len(set(outcomes.values())) > 1:
+                divergent.setdefault(filter_name, []).append((probe["v"], outcomes))
+
+    assert not divergent, (
+        "these filters behave differently depending on which environment "
+        "renders them: "
+        + "; ".join(
+            f"{name} on {cases[0][0]!r} -> "
+            + ", ".join(f"{n}={o!r}" for n, o in cases[0][1].items())
+            for name, cases in divergent.items()
+        )
+    )
+
+
+# ---------------------------------------------------------------------------
+# End to end: what a real run actually writes to a file
+# ---------------------------------------------------------------------------
+
+@pytest.mark.e2e
+@pytest.mark.parametrize(
+    "expression,expected",
+    [
+        ("{{ zero | default('MISSING') }}", "0"),
+        ("{{ blank | default('MISSING') }}", ""),
+        ("{{ absent | default('MISSING') }}", "MISSING"),
+    ],
+    ids=["zero_is_not_missing", "empty_is_not_missing", "absent_is_missing"],
+)
+def test_the_cli_writes_what_the_runtime_renders(expression, expected, tmp_path):
+    """The environments agreeing is only worth something if the file agrees."""
+    pipeline = tmp_path / "p.yaml"
+    pipeline.write_text(
+        f"""
+id: conformance
+name: Conformance
+parameters:
+  zero:
+    type: integer
+    default: 0
+  blank:
+    type: string
+    default: ""
+steps:
+  - id: write_it
+    tool: filesystem
+    action: write
+    parameters:
+      path: "./out.txt"
+      content: "{expression}"
+"""
+    )
+
+    env = dict(os.environ)
+    env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "")
+    env.pop("ANTHROPIC_API_KEY", None)
+    env["ORCHESTRATOR_AUTO_INSTALL"] = "0"
+    result = subprocess.run(
+        [sys.executable, "-m", "orchestrator.cli", "run", str(pipeline)],
+        cwd=str(tmp_path),
+        env=env,
+        capture_output=True,
+        text=True,
+        timeout=300,
+    )
+
+    assert result.returncode == 0, (
+        f"{expression} did not run: {result.stdout[-800:]}{result.stderr[-800:]}"
+    )
+    assert (tmp_path / "out.txt").read_text() == expected

tests/test_validator_agrees_with_runtime.py

@@ -93,17 +93,22 @@ def test_every_environment_knows_every_filter_the_runtime_registers():
     "expression",
     [
         "{{ topic | slugify }}",
-        "{{ topic | from_json }}",
         "{{ '/a/b/c.txt' | basename }}",
         "{{ topic | regex_search('World') }}",
         "{{ topic | truncate_words(2) }}",
+        # Valid JSON, because the subject is whether the filter is *known*.
+        # This case used to feed `from_json` the literal "Hello World Report",
+        # which is not JSON, so the pipeline failed for an unrelated reason and
+        # the assertion below was satisfied by the wrong error.
+        "{{ '[1, 2]' | from_json }}",
     ],
 )
 def test_a_pipeline_using_a_runtime_filter_validates(expression):
-    error = _validate(_pipeline(expression))
-
-    assert error is None or "filter" not in error.lower(), (
-        f"{expression} was rejected over its filter: {error}"
+    # Assert it compiles. The previous form accepted any error whose text did
+    # not contain "filter", so an unrelated rendering failure passed as though
+    # the filter had been recognised.
+    assert _validate(_pipeline(expression)) is None, (
+        f"{expression} does not validate, though the runtime registers this filter"
     )