diff --git a/docs/template_globals.md b/docs/template_globals.md new file mode 100644 index 0000000..f47051d --- /dev/null +++ b/docs/template_globals.md @@ -0,0 +1,61 @@ + + +# Template globals + +A pipeline template can call these functions: + +```yaml +- id: save + tool: filesystem + action: write + parameters: + path: "report-{{ execution.timestamp }}.md" + content: "{{ include_file('header.md') }}" +``` + +They are **not** filters. A filter transforms a value the author already holds, +so any Jinja environment can evaluate one. Each global below answers a question +about the state of a *run*, so only the runtime can evaluate it: + +```yaml +- id: make_it # writes ./artifact +- id: check_it # content: "{{ file_exists('artifact') }}" -> True +``` + +`check_it` renders `True` because by then `make_it` has run. The compiler and +the validators therefore know these names but hold none of the implementations +-- if they held them, that expression would be answered at compile time, before +any step had run, and would write `False` into the file with nothing failing. + +## Calling them + +A global must be **called**. Naming one without calling it renders the function +object itself: + +| Written | Result | +|-|-| +| `{{ now() }}` | the time | +| `{{ now }}` | `` written into your artifact | +| `{{ now.foo }}` | fails at run time | +| `{{ now(1, 2) }}` | fails at run time | + +All three of the wrong forms are refused at compile time (`orchestrator +validate`, exit 2). + + +## Available globals + +| Global | Arguments | Description | +|-|-|-| +| `active_loops` | 0 | The names of the loops currently running. | +| `current_loop_name` | 0 | The innermost active loop's name. | +| `file_exists` | 1 to 2 | Whether a path exists, answered when the step runs, so a file an earlier step wrote counts. | +| `historical_loops` | 0 | The names of finished loops whose values are still reachable. | +| `include_file` | 1 to 2 | The contents of a file, read when the step runs. | +| `loop_item_at` | 2 | An item at a fixed index of a named loop: `loop_item_at('outer', 0)`. | +| `loop_var` | 2 | A named loop's variable by name: `loop_var('outer', 'item')`. | +| `now` | 0 | The current time. Re-evaluated at every use, so two steps in one run disagree -- prefer `execution.timestamp` where a run needs one answer. | + +A name that is not on this list is not a global. `{{ nowx() }}` is a +typo and is refused at compile time rather than becoming an undefined +value at run time. diff --git a/scripts/generate_template_globals_docs.py b/scripts/generate_template_globals_docs.py new file mode 100644 index 0000000..240d90a --- /dev/null +++ b/scripts/generate_template_globals_docs.py @@ -0,0 +1,114 @@ +#!/usr/bin/env python3 +"""Generate docs/template_globals.md from the pipeline-global registry. + +The page is written from the same specs the validator checks calls against, so +it cannot document a global that does not exist, miss one that does, or state +an argument count the validator would reject. +`tests/test_template_globals.py` re-runs this and fails if the committed file +differs. + + python scripts/generate_template_globals_docs.py # write the page + python scripts/generate_template_globals_docs.py --check # exit 1 if stale +""" + +from __future__ import annotations + +import argparse +import pathlib +import sys + +ROOT = pathlib.Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT / "src")) + +from orchestrator.core.template_globals import GLOBAL_SPECS # noqa: E402 + +TARGET = ROOT / "docs" / "template_globals.md" + +HEADER = """ + +# Template globals + +A pipeline template can call these functions: + +```yaml +- id: save + tool: filesystem + action: write + parameters: + path: "report-{{ execution.timestamp }}.md" + content: "{{ include_file('header.md') }}" +``` + +They are **not** filters. A filter transforms a value the author already holds, +so any Jinja environment can evaluate one. Each global below answers a question +about the state of a *run*, so only the runtime can evaluate it: + +```yaml +- id: make_it # writes ./artifact +- id: check_it # content: "{{ file_exists('artifact') }}" -> True +``` + +`check_it` renders `True` because by then `make_it` has run. The compiler and +the validators therefore know these names but hold none of the implementations +-- if they held them, that expression would be answered at compile time, before +any step had run, and would write `False` into the file with nothing failing. + +## Calling them + +A global must be **called**. Naming one without calling it renders the function +object itself: + +| Written | Result | +|-|-| +| `{{ now() }}` | the time | +| `{{ now }}` | `` written into your artifact | +| `{{ now.foo }}` | fails at run time | +| `{{ now(1, 2) }}` | fails at run time | + +All three of the wrong forms are refused at compile time (`orchestrator +validate`, exit 2). + +""" + + +def render() -> str: + lines = [HEADER, "## Available globals\n"] + lines.append("| Global | Arguments | Description |") + lines.append("|-|-|-|") + for spec in sorted(GLOBAL_SPECS, key=lambda s: s.name): + lines.append(f"| `{spec.name}` | {spec.arity} | {spec.summary} |") + lines.append("") + lines.append( + "A name that is not on this list is not a global. `{{ nowx() }}` is a\n" + "typo and is refused at compile time rather than becoming an undefined\n" + "value at run time.\n" + ) + return "\n".join(lines) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--check", action="store_true", help="verify, do not write") + args = parser.parse_args() + + rendered = render() + if args.check: + current = TARGET.read_text() if TARGET.exists() else "" + if current != rendered: + print(f"{TARGET.relative_to(ROOT)} is out of date.", file=sys.stderr) + print( + "Regenerate with: python scripts/generate_template_globals_docs.py", + file=sys.stderr, + ) + return 1 + print(f"{TARGET.relative_to(ROOT)} is up to date.") + return 0 + + TARGET.parent.mkdir(parents=True, exist_ok=True) + TARGET.write_text(rendered) + print(f"wrote {TARGET.relative_to(ROOT)}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/orchestrator/core/template_globals.py b/src/orchestrator/core/template_globals.py new file mode 100644 index 0000000..9a1eecf --- /dev/null +++ b/src/orchestrator/core/template_globals.py @@ -0,0 +1,206 @@ +"""The pipeline language's global functions, and how they may be called. + +Filters transform a value the author already holds. Globals do not: each of +these answers a question about the state of a *run* -- what time it started, +whether a file exists yet, which loop iteration this is. That is why only the +runtime holds their implementations, and why the compiler and the validators +must know them by name without being able to call them. `template_sandbox` +explains the mechanics; this module says what the names *are*. + +#450 taught both validators these names, which stopped `{{ now() }}` -- a +pipeline that runs correctly -- from being reported as an undefined variable. +It stopped there, at the name, and so accepted every way of naming a global +that is not actually a call:: + + {{ nowx() }} rejected (not a global) + {{ now.foo }} accepted -> fails at run time + {{ file_exists.bad }} accepted -> fails at run time + {{ now(1, 2, 3) }} accepted -> fails at run time + {{ file_exists() }} accepted -> fails at run time + {{ now }} accepted -> *runs*, and writes + "" + +The last one is the worst, because nothing fails: the repr of a live function +object is written into the artifact as though it were data. (It is not a +sandbox escape -- `now.__globals__` and `now.__class__` are both refused by +`SandboxedEnvironment`, which #447 put in place. What leaks is the repr, not +the object graph.) + +So the contract has to cover the call, not just the name, and it is declared +here rather than inferred from the callables. Inference was the right move in +#450, when the only question was which names exist and drift was the risk. It +cannot express what this needs -- an argument contract, a summary, later a +deprecation -- and it makes the public language a shadow of a private +implementation detail. `test_template_globals.py` asserts every spec matches +the callable the runtime actually registers, which keeps the drift protection +that derivation gave for free. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, FrozenSet, List, Optional, Tuple + + +@dataclass(frozen=True) +class GlobalSpec: + """One global function: its name, how many arguments it takes, what it does.""" + + name: str + min_args: int + max_args: Optional[int] # None means unbounded + summary: str + + def accepts(self, positional: int) -> bool: + if positional < self.min_args: + return False + return self.max_args is None or positional <= self.max_args + + @property + def arity(self) -> str: + """How the argument count reads in an error message or a doc table.""" + if self.max_args is None: + return f"{self.min_args} or more" + if self.min_args == self.max_args: + return str(self.min_args) + return f"{self.min_args} to {self.max_args}" + + +#: Every global the pipeline language offers. The runtime registers exactly +#: these; the compiler and validators recognise exactly these. +GLOBAL_SPECS: Tuple[GlobalSpec, ...] = ( + GlobalSpec( + "now", 0, 0, + "The current time. Re-evaluated at every use, so two steps in one run " + "disagree -- prefer `execution.timestamp` where a run needs one answer.", + ), + GlobalSpec( + "file_exists", 1, 2, + "Whether a path exists, answered when the step runs, so a file an " + "earlier step wrote counts.", + ), + GlobalSpec( + "include_file", 1, 2, + "The contents of a file, read when the step runs.", + ), + GlobalSpec( + "loop_var", 2, 2, + "A named loop's variable by name: `loop_var('outer', 'item')`.", + ), + GlobalSpec( + "loop_item_at", 2, 2, + "An item at a fixed index of a named loop: `loop_item_at('outer', 0)`.", + ), + GlobalSpec("current_loop_name", 0, 0, "The innermost active loop's name."), + GlobalSpec("active_loops", 0, 0, "The names of the loops currently running."), + GlobalSpec( + "historical_loops", 0, 0, + "The names of finished loops whose values are still reachable.", + ), +) + +GLOBAL_NAMES: FrozenSet[str] = frozenset(spec.name for spec in GLOBAL_SPECS) + +_BY_NAME = {spec.name: spec for spec in GLOBAL_SPECS} + + +def global_spec(name: str) -> Optional[GlobalSpec]: + """The spec for `name`, or None if it is not a pipeline global.""" + return _BY_NAME.get(name) + + +#: Stable identifiers for the two ways a global can be misused. Callers match +#: on these rather than on message text. +NOT_CALLED = "global_not_called" +WRONG_ARITY = "global_wrong_arity" + + +@dataclass(frozen=True) +class GlobalMisuse: + """A global named in a template in a way that cannot work at run time.""" + + name: str + code: str + message: str + suggestion: str + + +def find_global_misuse(ast: Any) -> List[GlobalMisuse]: + """Every misuse of a pipeline global in a parsed template. + + Works on the parsed AST rather than on the text because the text does not + distinguish the cases: `now` appears identically in `{{ now() }}`, + `{{ now.foo }}` and `{{ now }}`, and only the first is a call. + """ + from jinja2 import nodes + + # A Name node is a legitimate use only when it is the thing being called. + # Identity matters here, not the name: `{{ now() and now.foo }}` has two + # Name nodes spelled the same, one valid and one not. + calls_by_callee = { + id(call.node): call + for call in ast.find_all(nodes.Call) + if isinstance(call.node, nodes.Name) and call.node.name in GLOBAL_NAMES + } + + # `{% for now in items %}{{ now }}{% endfor %}` rebinds the name: the + # target is a `store`, but the use inside the body is an ordinary `load` + # and is indistinguishable from ours without tracking scope. A template + # that binds the name anywhere is left alone entirely -- deliberately + # conservative, because a false rejection here is the exact failure the + # last three changes to this validator existed to remove. + shadowed = { + node.name + for node in ast.find_all(nodes.Name) + if getattr(node, "ctx", "load") != "load" + } + + misuse: List[GlobalMisuse] = [] + seen = set() + for name_node in ast.find_all(nodes.Name): + spec = global_spec(name_node.name) + if spec is None or spec.name in shadowed: + continue + if getattr(name_node, "ctx", "load") != "load": + continue + + call = calls_by_callee.get(id(name_node)) + if call is None: + key = (spec.name, NOT_CALLED) + if key in seen: + continue + seen.add(key) + call_form = f"{spec.name}()" if spec.min_args == 0 else f"{spec.name}(...)" + misuse.append(GlobalMisuse( + name=spec.name, + code=NOT_CALLED, + message=( + f"'{spec.name}' is a function and must be called: write " + f"'{call_form}'. Naming it without calling it yields the " + f"function itself, which renders as ''." + ), + suggestion=call_form, + )) + continue + + # `f(*args)` cannot be counted before it runs, so it is not checked. + if call.dyn_args is not None: + continue + + positional = len(call.args) + if not spec.accepts(positional): + key = (spec.name, WRONG_ARITY, positional) + if key in seen: + continue + seen.add(key) + misuse.append(GlobalMisuse( + name=spec.name, + code=WRONG_ARITY, + message=( + f"'{spec.name}' takes {spec.arity} argument(s), not " + f"{positional}" + ), + suggestion=f"{spec.name} expects {spec.arity} argument(s)", + )) + + return misuse diff --git a/src/orchestrator/core/template_sandbox.py b/src/orchestrator/core/template_sandbox.py index c430fb7..cf743ca 100644 --- a/src/orchestrator/core/template_sandbox.py +++ b/src/orchestrator/core/template_sandbox.py @@ -88,14 +88,15 @@ def pipeline_global_names() -> FrozenSet[str]: pipelines that run correctly -- the false-positive class of #448 again. They import this instead, so the names are stated once. - Derived rather than hand-listed: a literal list drifts the moment someone - registers a ninth global, and drift is the failure this module exists to - prevent. + The names come from `template_globals.GLOBAL_SPECS`, which also carries + each one's argument contract. They were originally derived from what + `TemplateManager` happened to register, which could not drift but could not + express an arity either; `test_template_globals.py` compares the two, so + the drift protection survives the change. """ - from .template_manager import TemplateManager + from .template_globals import GLOBAL_NAMES - stock = frozenset(SandboxedEnvironment().globals) - return frozenset(TemplateManager().env.globals) - stock + return GLOBAL_NAMES def sandboxed_template(source: str, **kwargs: Any) -> Template: diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index 06d8b78..dd81b11 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/src/orchestrator/validation/template_validator.py @@ -17,6 +17,7 @@ from jinja2 import Environment, TemplateSyntaxError, meta from jinja2.sandbox import SandboxedEnvironment +from ..core.template_globals import find_global_misuse from ..core.template_sandbox import pipeline_global_names logger = logging.getLogger(__name__) @@ -326,10 +327,24 @@ def _validate_variables( try: # Parse template to get AST ast = self.env.parse(template) - + + # Knowing the name is not knowing the use. `{{ now }}` names a + # global correctly and still cannot work: it renders the function + # object itself, so the artifact receives "" and + # nothing fails. The AST is what tells a call apart from an + # attribute access or a bare mention; the text does not. + for misuse in find_global_misuse(ast): + errors.append(TemplateValidationError( + template=template, + error_type=misuse.code, + message=misuse.message, + context_path=context_path, + suggestions=[misuse.suggestion], + )) + # Find all variable references var_names = meta.find_undeclared_variables(ast) - + # Also look for loop variables manually (since they start with $) loop_var_matches = [] for loop_var in self.loop_vars: diff --git a/tests/test_template_globals.py b/tests/test_template_globals.py new file mode 100644 index 0000000..54adabe --- /dev/null +++ b/tests/test_template_globals.py @@ -0,0 +1,286 @@ +"""A global must be called, and called correctly. + +#450 taught the validators the eight global *names*, which stopped `{{ now() }}` +-- a pipeline that runs correctly -- from being reported as an undefined +variable. It stopped at the name. Every other way of writing the name was +accepted and then failed at run time, except the one that did not fail at all: + + {{ now }} -> validates, runs, and writes + ".now + at 0x1084...>" into the artifact + +Nothing errored. The repr of a live function object was written where the +author expected a timestamp. (It is not a sandbox escape: `now.__globals__` and +`now.__class__` are refused by the sandboxed environment #447 introduced. What +leaks is the repr, not the object graph -- and these tests pin that too.) + +These tests are the negative half of the contract: what the language refuses. +""" + +import inspect +import subprocess +import sys +from pathlib import Path + +import pytest + +from orchestrator.core.template_globals import ( + GLOBAL_NAMES, + GLOBAL_SPECS, + NOT_CALLED, + WRONG_ARITY, + find_global_misuse, +) +from orchestrator.core.template_manager import TemplateManager +from orchestrator.core.template_sandbox import pipeline_global_names + +pytestmark = [pytest.mark.contract] + +ROOT = Path(__file__).resolve().parent.parent + + +def _misuse(expression): + """The misuse codes a template's expression produces, as a set.""" + ast = TemplateManager().env.parse(expression) + return {m.code for m in find_global_misuse(ast)} + + +# --------------------------------------------------------------------------- +# The registry describes the functions the runtime actually registers +# --------------------------------------------------------------------------- + +def test_the_registry_matches_what_the_runtime_registers(): + """The drift check that derivation used to give for free. + + #450 derived the names from `TemplateManager`, so they could not disagree. + Declaring them buys an argument contract and costs that guarantee, unless + something asserts it -- this. + """ + registered = set(TemplateManager().env.globals) - set( + __import__("jinja2").sandbox.SandboxedEnvironment().globals + ) + assert GLOBAL_NAMES == registered, ( + f"the declared language and the runtime disagree: " + f"declared_only={sorted(GLOBAL_NAMES - registered)}, " + f"registered_only={sorted(registered - GLOBAL_NAMES)}" + ) + + +@pytest.mark.parametrize("spec", GLOBAL_SPECS, ids=lambda s: s.name) +def test_each_declared_arity_matches_the_real_signature(spec): + """A declared contract that does not match the callable is worse than none. + + It would reject calls that work, or accept calls that cannot. + """ + func = TemplateManager().env.globals[spec.name] + signature = inspect.signature(func) + + positional = [ + p for p in signature.parameters.values() + if p.kind in (p.POSITIONAL_ONLY, p.POSITIONAL_OR_KEYWORD) + ] + required = sum(1 for p in positional if p.default is p.empty) + unbounded = any(p.kind is p.VAR_POSITIONAL for p in signature.parameters.values()) + + assert spec.min_args == required, ( + f"{spec.name} declares {spec.min_args} required argument(s) but its " + f"signature {signature} requires {required}" + ) + if unbounded: + assert spec.max_args is None, f"{spec.name} takes *args but declares a maximum" + else: + assert spec.max_args == len(positional), ( + f"{spec.name} declares a maximum of {spec.max_args} but its " + f"signature {signature} accepts {len(positional)}" + ) + + +def test_pipeline_global_names_comes_from_the_registry(): + assert pipeline_global_names() == GLOBAL_NAMES + + +# --------------------------------------------------------------------------- +# What the language refuses +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "expression,expected", + [ + # Named but never called: renders the function object itself. + ("{{ now }}", NOT_CALLED), + ("{{ current_loop_name }}", NOT_CALLED), + # Attribute access on the function object. + ("{{ now.foo }}", NOT_CALLED), + ("{{ file_exists.bad }}", NOT_CALLED), + ("{{ active_loops.x.y }}", NOT_CALLED), + # Indexing it. + ("{{ now[0] }}", NOT_CALLED), + ("{{ active_loops['a'] }}", NOT_CALLED), + # Passed somewhere as a value. + ("{{ [1, 2] | map(now) | list }}", NOT_CALLED), + ("{% if now %}x{% endif %}", NOT_CALLED), + # Called with the wrong number of arguments. + ("{{ now(1) }}", WRONG_ARITY), + ("{{ now(1, 2, 3) }}", WRONG_ARITY), + ("{{ file_exists() }}", WRONG_ARITY), + ("{{ file_exists('a', 'b', 'c') }}", WRONG_ARITY), + ("{{ loop_var('only_one') }}", WRONG_ARITY), + ("{{ current_loop_name('unexpected') }}", WRONG_ARITY), + ], +) +def test_misuse_is_detected(expression, expected): + assert expected in _misuse(expression), ( + f"{expression} is not a usable call and was accepted" + ) + + +@pytest.mark.parametrize( + "expression", + [ + "{{ now() }}", + "{{ current_loop_name() }}", + "{{ active_loops() }}", + "{{ file_exists('a.txt') }}", + "{{ file_exists('a.txt', '/base') }}", + "{{ include_file('a.txt') }}", + "{{ loop_var('outer', 'item') }}", + "{{ loop_item_at('outer', 0) }}", + # Keyword arguments are the callable's business, not the arity check's. + "{{ file_exists('a.txt', base_dir='/base') }}", + # Nested in a larger expression. + "{{ 'yes' if file_exists('a.txt') else 'no' }}", + "{{ now() | string | upper }}", + # A name that merely contains a global's name is not that global. + "{{ nowhere }}", + "{{ my_now }}", + ], +) +def test_valid_use_is_left_alone(expression): + assert _misuse(expression) == set(), f"{expression} is valid and was rejected" + + +@pytest.mark.parametrize( + "expression", + [ + "{% for now in items %}{{ now }}{% endfor %}", + "{% set now = 'x' %}{{ now }}", + "{% macro f(now) %}{{ now }}{% endmacro %}", + ], +) +def test_a_rebound_name_is_not_our_global(expression): + """A template that binds the name is talking about its own variable. + + The binding site is a `store`, but the use is an ordinary `load` and looks + exactly like ours. Rejecting it would be a false positive of the kind the + last three changes here existed to remove. + """ + assert _misuse(expression) == set() + + +def test_one_expression_can_hold_a_valid_and_an_invalid_use(): + """Identity, not spelling: two `now` nodes, only one of them a call.""" + assert NOT_CALLED in _misuse("{{ now() }}{{ now.foo }}") + + +# --------------------------------------------------------------------------- +# The validator actually asks +# --------------------------------------------------------------------------- + +def _compile(expression): + """Compile the way `orchestrator validate` does. Returns None or the error.""" + import asyncio + + from orchestrator.compiler.yaml_compiler import YAMLCompiler + + pipeline = f""" +id: probe +name: Probe +steps: + - id: write_it + tool: filesystem + action: write + parameters: + path: "./out.txt" + content: "{expression}" +""" + try: + asyncio.run(YAMLCompiler().compile(pipeline, {})) + return None + except Exception as exc: # noqa: BLE001 - the message is the subject + return str(exc) + + +@pytest.mark.parametrize( + "expression", + ["{{ now }}", "{{ now.foo }}", "{{ file_exists() }}", "{{ now(1, 2, 3) }}"], +) +def test_the_validator_refuses_a_misused_global(expression): + """The checks above test the function; this tests that anything calls it. + + Without this, deleting the call site in the template validator leaves every + test in this file passing. + """ + assert _compile(expression) is not None, ( + f"{expression} compiles, so the misuse check is not wired into validation" + ) + + +@pytest.mark.parametrize( + "expression", ["{{ now() }}", "{{ file_exists('a.txt') }}"] +) +def test_the_validator_accepts_a_proper_call(expression): + assert _compile(expression) is None, f"{expression} is valid and was refused" + + +# --------------------------------------------------------------------------- +# The function object itself must stay out of reach +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "expression", + [ + "{{ now.__globals__ }}", + "{{ now.__class__ }}", + "{{ now.__call__() }}", + "{{ now.__globals__['__builtins__'] }}", + ], +) +def test_a_global_is_not_a_way_out_of_the_sandbox(expression): + """`{{ now }}` puts a live function in reach of the template. + + The sandbox refuses to traverse it, and that is what keeps this a + correctness bug rather than the #447 escape all over again. Rendering, not + validating, is the subject here: this must hold even for a template that + never went through the validator. + """ + from jinja2.exceptions import SecurityError, UndefinedError + + env = TemplateManager().env + with pytest.raises((SecurityError, UndefinedError)): + env.from_string(expression).render() + + +# --------------------------------------------------------------------------- +# Documentation +# --------------------------------------------------------------------------- + +def test_the_generated_globals_docs_are_committed_and_current(): + result = subprocess.run( + [sys.executable, str(ROOT / "scripts" / "generate_template_globals_docs.py"), + "--check"], + cwd=str(ROOT), + capture_output=True, + text=True, + timeout=120, + ) + assert result.returncode == 0, ( + f"docs/template_globals.md is out of date. Run:\n" + f" python scripts/generate_template_globals_docs.py\n\n" + f"{result.stdout}\n{result.stderr}" + ) + + +def test_every_global_appears_in_the_documentation(): + text = (ROOT / "docs" / "template_globals.md").read_text() + for spec in GLOBAL_SPECS: + assert f"`{spec.name}`" in text, f"{spec.name} is undocumented"