diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2d12e03..dec175de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -177,9 +177,16 @@ jobs: # everyone else's fixture (#431). This is the guard that keeps it fixed. if: always() run: | - if ! git diff --quiet; then - echo "::error::The test suite modified tracked files:" - git diff --stat + # --untracked-files=all, not `git diff`: a diff only sees + # modifications to tracked files, so a test could still scatter new + # files through the checkout and the guard would pass. + # legacy-results.txt is written by the step above, on purpose. It is + # the only expected artifact; anything else is a test misbehaving. + dirt=$(git status --porcelain --untracked-files=all \ + | grep -v ' legacy-results\.txt$' || true) + if [ -n "$dirt" ]; then + echo "::error::The test suite left the repository dirty:" + echo "$dirt" echo echo "Tests must write generated files to a temp directory" echo "(pytest's tmp_path), never into the repository." diff --git a/docs/actions.md b/docs/actions.md new file mode 100644 index 00000000..c4f6069e --- /dev/null +++ b/docs/actions.md @@ -0,0 +1,83 @@ + + +# Action vocabulary + +A step names **either** a tool and an operation on it: + +```yaml +- id: save + tool: filesystem + action: write + parameters: + path: out/report.md + content: "..." +``` + +**or** an action the runtime executes itself: + +```yaml +- id: think + action: generate + parameters: + prompt: "Summarise the findings" +``` + +This page lists the second group. It is generated from +`orchestrator.core.actions`, the single registry the executor dispatches on and +the validator recognises, so it cannot fall out of step with the code. + +An action that appears nowhere on this page is **refused**. It is not turned +into a prompt for the model: `action: gernate` is a typo and fails, at compile +time and again at dispatch. + +## When does a pipeline fail, and with which exit code? + +| Situation | Detected | Exit code | +|-|-|-| +| Malformed YAML, unknown action, missing required parameter, dependency on a step that does not exist, template that can never resolve | compile time (`orchestrator validate`) | **2** | +| A reference expected to resolve from an earlier step's result, still unresolved at the moment a tool would use it | run time, before any side effect | **1** | +| A step raising during execution | run time | **1** | + +The distinction matters because template resolution runs in several passes. A +reference to a prior step cannot be resolved before that step has run, so it is +not a compile error -- but it must not survive to the point where a value is +written to a file or sent to a model. At that boundary it fails the step, no +output is written, and the run exits 1. + + +## Actions + +| Action | Requires | Required parameters | Aliases | Description | +|-|-|-|-|-| +| `action_loop` | — | — | — | Run a sequence of actions repeatedly until a condition holds. | +| `analyze_text` | model | — | `analyze` | Analyse supplied text with the selected model. | +| `capture_result` | — | — | — | Capture a prior step's result under a new name. | +| `control_flow` | — | — | — | Control-flow marker step. | +| `create_parallel_queue` | — | — | — | Fan work out across a parallel queue. | +| `evaluate_condition` | — | `condition` | — | Evaluate a condition expression and record the verdict. | +| `filesystem` | tool | — | `file` | Filesystem operation named by the step's `action` parameter. | +| `generate` | model | `prompt` | `generate-text`, `generate_text` | Generate text from a prompt using the selected model. | +| `generate_structured` | model | `prompt`, `schema` | `generate-structured` | Generate an object conforming to a declared JSON Schema. | +| `loop_complete` | — | — | — | Marker emitted when a loop finishes. | +| `process` | tool | — | — | Transform data with the deterministic data-processing tool. | +| `validate` | tool | — | — | Validate data against a schema with the validation tool. | + +Aliases are accepted but deprecated: the compiler rewrites them to the +canonical name, emits a `DeprecationWarning`, and only the canonical +spelling appears in the task graph and the execution trace. + +## Prose families + +Two shapes are matched by pattern rather than by name, because they are +written as instructions rather than identifiers. + +| Family | Description | +|-|-| +| `auto` | An explicit request for the model to interpret the instruction, e.g. `action: summarise the findings`. | +| `echo` | Print a message, e.g. `action: echo "starting"`. | +| `file_prose` | Filesystem instruction written as prose, e.g. `action: write the following content to report.md`. | + +`...` is the one case where an instruction is deliberately +handed to the model to interpret. That is an explicit request, which is why +it remains supported while the implicit "unrecognised action becomes a +prompt" behaviour does not. diff --git a/docs/adr/0001-product-contract.md b/docs/adr/0001-product-contract.md index cfd8cf9e..5081851d 100644 --- a/docs/adr/0001-product-contract.md +++ b/docs/adr/0001-product-contract.md @@ -118,10 +118,34 @@ Exit codes: `0` success, `1` execution failure, `2` validation/compile failure, 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). +`core/actions.py` is the single source of truth for the second group. Each +action is an `ActionSpec` carrying its canonical name, aliases, handler, +whether it needs a model or a tool, its required parameters and its result +schema. Everything else is *derived* from that registry rather than restated +beside it: + +| Consumer | Derived as | +|-|-| +| Executor dispatch | `resolve_action(...).handler` | +| Validator recognition | `is_known_action(...)` | +| Advertised `supported_actions` | `SUPPORTED_ACTIONS` | +| Alias normalisation | `canonical_action(...)`, applied by the compiler | +| Documentation | `docs/actions.md`, generated and drift-tested | + +So `validate` and `run` cannot disagree about whether an action exists (#241), +and the vocabulary cannot drift apart again. + +**An unrecognised action is refused.** It used to become a prompt for the +model, so `action: gernate` returned a plausible answer and reported success. +It now fails at compile time, and again at dispatch — the runtime checks +independently, because a caller can construct a `Task` and reach execution +without going through YAML validation at all. `...` remains +supported: that is an author explicitly asking the model to interpret an +instruction, which is not the same as a typo falling through. + +Aliases are accepted but deprecated. The compiler rewrites them to the +canonical name and warns, so exactly one spelling reaches the task graph and +the trace. Template rendering deliberately falls back to returning the original text when a reference is undefined, because resolution runs in several passes and a diff --git a/scripts/generate_action_docs.py b/scripts/generate_action_docs.py new file mode 100644 index 00000000..80275228 --- /dev/null +++ b/scripts/generate_action_docs.py @@ -0,0 +1,134 @@ +#!/usr/bin/env python3 +"""Generate docs/actions.md from the action registry. + +The vocabulary is documented from the same object the executor dispatches on, +so the documentation cannot describe an action that does not exist or miss one +that does. `tests/test_action_contract_docs.py` re-runs this and fails if the +committed file differs, which is what stops the two drifting apart. + + python scripts/generate_action_docs.py # write docs/actions.md + python scripts/generate_action_docs.py --check # exit 1 if out of date +""" + +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.actions import ACTION_FAMILIES, ACTION_SPECS # noqa: E402 + +TARGET = ROOT / "docs" / "actions.md" + +HEADER = """ + +# Action vocabulary + +A step names **either** a tool and an operation on it: + +```yaml +- id: save + tool: filesystem + action: write + parameters: + path: out/report.md + content: "..." +``` + +**or** an action the runtime executes itself: + +```yaml +- id: think + action: generate + parameters: + prompt: "Summarise the findings" +``` + +This page lists the second group. It is generated from +`orchestrator.core.actions`, the single registry the executor dispatches on and +the validator recognises, so it cannot fall out of step with the code. + +An action that appears nowhere on this page is **refused**. It is not turned +into a prompt for the model: `action: gernate` is a typo and fails, at compile +time and again at dispatch. + +## When does a pipeline fail, and with which exit code? + +| Situation | Detected | Exit code | +|-|-|-| +| Malformed YAML, unknown action, missing required parameter, dependency on a step that does not exist, template that can never resolve | compile time (`orchestrator validate`) | **2** | +| A reference expected to resolve from an earlier step's result, still unresolved at the moment a tool would use it | run time, before any side effect | **1** | +| A step raising during execution | run time | **1** | + +The distinction matters because template resolution runs in several passes. A +reference to a prior step cannot be resolved before that step has run, so it is +not a compile error -- but it must not survive to the point where a value is +written to a file or sent to a model. At that boundary it fails the step, no +output is written, and the run exits 1. + +""" + + +def render() -> str: + lines = [HEADER, "## Actions\n"] + lines.append("| Action | Requires | Required parameters | Aliases | Description |") + lines.append("|-|-|-|-|-|") + for spec in sorted(ACTION_SPECS, key=lambda s: s.name): + aliases = ", ".join(f"`{a}`" for a in sorted(spec.aliases)) or "—" + required = ", ".join(f"`{p}`" for p in sorted(spec.required_parameters)) or "—" + requires = "—" if spec.requires == "none" else spec.requires + lines.append( + f"| `{spec.name}` | {requires} | {required} | {aliases} | {spec.summary} |" + ) + + lines.append("") + lines.append("Aliases are accepted but deprecated: the compiler rewrites them to the") + lines.append("canonical name, emits a `DeprecationWarning`, and only the canonical") + lines.append("spelling appears in the task graph and the execution trace.") + lines.append("") + + lines.append("## Prose families\n") + lines.append( + "Two shapes are matched by pattern rather than by name, because they are\n" + "written as instructions rather than identifiers.\n" + ) + lines.append("| Family | Description |") + lines.append("|-|-|") + for family in sorted(ACTION_FAMILIES, key=lambda f: f.name): + lines.append(f"| `{family.name}` | {family.summary} |") + lines.append("") + lines.append( + "`...` is the one case where an instruction is deliberately\n" + "handed to the model to interpret. That is an explicit request, which is why\n" + "it remains supported while the implicit \"unrecognised action becomes a\n" + "prompt\" behaviour does not.\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_action_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/scripts/utilities/organization_maintenance.py b/scripts/utilities/organization_maintenance.py index f6433e9d..183eae6d 100644 --- a/scripts/utilities/organization_maintenance.py +++ b/scripts/utilities/organization_maintenance.py @@ -849,7 +849,7 @@ def generate_maintenance_documentation(self) -> Path: "", f"## System Status (as of {datetime.now().strftime('%Y-%m-%d %H:%M:%S')})", "", - ] + ]) # Add current system status try: diff --git a/src/orchestrator/compiler/yaml_compiler.py b/src/orchestrator/compiler/yaml_compiler.py index 2e5929ed..345fcd42 100644 --- a/src/orchestrator/compiler/yaml_compiler.py +++ b/src/orchestrator/compiler/yaml_compiler.py @@ -4,11 +4,13 @@ import logging import re +import warnings from typing import Any, Dict, List, Optional import yaml from jinja2 import Environment, StrictUndefined +from ..core.actions import canonical_action from ..core.pipeline import Pipeline from ..core.task import Task from ..core.template_metadata import TemplateMetadata @@ -247,6 +249,17 @@ async def compile( if self.validation_report.has_errors and self.validation_level == ValidationLevel.STRICT: # Create comprehensive error message from validation report error_message = self.validation_report.format_report(format_type=OutputFormat.SUMMARY) + # The SUMMARY format reports counts by category -- "1 + # errors, tool: 1" -- which tells an author that something + # is wrong but not what. Lead with the actual messages so + # the exception alone is enough to fix the pipeline. + detail = "\n".join( + f" - {issue.message}" + for issue in self.validation_report.issues + if issue.severity is ValidationSeverity.ERROR + ) + if detail: + error_message = f"{detail}\n\n{error_message}" raise YAMLCompilerError(f"Pipeline validation failed:\n{error_message}") # Log validation summary @@ -1389,6 +1402,26 @@ def _build_task(self, task_def: Dict[str, Any], available_steps: List[str]) -> T metadata["is_while_loop"] = True metadata["while_condition"] = task_def["while"] + # Normalise action aliases so exactly one spelling reaches the task + # graph, the trace and every downstream consumer. Steps that name a + # `tool:` are left alone -- their `action` is an operation on that + # tool, not an entry in the action vocabulary. + if isinstance(action, str) and "tool" not in task_def: + canonical = canonical_action(action) + if canonical is not None and canonical != action.strip().lower(): + warnings.warn( + f"Task '{task_id}': action '{action}' is a deprecated " + f"alias of '{canonical}'. Support for the alias will be " + f"removed; write '{canonical}' instead.", + DeprecationWarning, + stacklevel=2, + ) + logger.warning( + "Task '%s': normalising deprecated action alias '%s' -> '%s'", + task_id, action, canonical, + ) + action = canonical + # Analyze templates in parameters template_metadata = self._analyze_parameter_templates(parameters, available_steps) diff --git a/src/orchestrator/control_systems/hybrid_control_system.py b/src/orchestrator/control_systems/hybrid_control_system.py index ed925d29..a681c7ea 100644 --- a/src/orchestrator/control_systems/hybrid_control_system.py +++ b/src/orchestrator/control_systems/hybrid_control_system.py @@ -7,7 +7,12 @@ from datetime import datetime from .model_based_control_system import ModelBasedControlSystem -from ..core.actions import BUILTIN_ACTION_HANDLERS +from ..core.actions import ( + SUPPORTED_ACTIONS, + match_action_family, + resolve_action, +) +from ..core.exceptions import UnknownActionError from ..core.task import Task from ..core.action_loop_task import ActionLoopTask from ..core.expressions import ( @@ -70,28 +75,9 @@ def __init__( if config is None: config = { "capabilities": { - "supported_actions": [ - # Model-based actions - "generate", - "analyze", - "transform", - "execute", - "search", - "extract", - "filter", - "synthesize", - "create", - "validate", - "optimize", - "review", - # File operations - "save_output", - "save_to_file", - "write_file", - "read_file", - "save", - "write", - ], + # Generated from core/actions.py, so this can no + # longer advertise an action nobody implements. + "supported_actions": list(SUPPORTED_ACTIONS), "parallel_execution": True, "streaming": True, "checkpoint_support": True, @@ -167,22 +153,34 @@ async def _execute_task_impl(self, task: Task, context: Dict[str, Any]) -> Any: # keeping a parallel chain of `if action_str == ...` branches -- is # what stops it from drifting out of step with the validator, which # accepts exactly the same names (#241). - handler_name = BUILTIN_ACTION_HANDLERS.get(action_str.strip()) - if handler_name: - logger.debug("Routing to built-in action handler: %s", handler_name) - return await getattr(self, handler_name)(task, context) - - # The remaining two families are matched by pattern, not by name: an - # action like "echo hello" or "write the following content to report.md" - # is prose, so there is no name to register. - if self._is_echo_operation(action_str): - return await self._handle_echo_operation(task, context) + spec = resolve_action(action_str) + if spec is not None: + if spec.handler is None: + # Registered, but run one level down -- `generate_structured` + # is ModelBasedControlSystem's. Falling through to the raise + # below would reject a perfectly valid action. + logger.debug("Delegating registered action %r to the model layer", spec.name) + return await super()._execute_task_impl(task, context) + logger.debug("Routing to built-in action handler: %s", spec.handler) + return await getattr(self, spec.handler)(task, context) - if self._is_file_operation(action_str): - return await self._handle_file_operation(task, context) + # The remaining families are matched by pattern, not by name: "echo + # hello", "write the following content to report.md", and an explicit + # ... instruction are prose, so there is no name to + # register. `auto` carries no handler -- it is the one case that is + # *meant* to reach the model. + family = match_action_family(str(task.action)) + if family is not None: + if family.handler is None: + logger.debug("Routing %r to the model via the 'auto' family", task.action) + return await super()._execute_task_impl(task, context) + logger.debug("Routing to action family %r: %s", family.name, family.handler) + return await getattr(self, family.handler)(task, context) - # Otherwise use model-based execution - return await super()._execute_task_impl(task, context) + # Nothing can run this. Previously execution fell through to the model, + # which turned the action itself into a prompt -- so `action: gernate` + # became a successful model call rather than a typo (#241 follow-up). + raise UnknownActionError(str(task.action), task_id=task.id) async def _handle_tool_execution(self, task: Task, tool_name: str, context: Dict[str, Any]) -> Any: """Handle execution with a specific tool.""" @@ -223,42 +221,6 @@ async def _handle_tool_execution(self, task: Task, tool_name: str, context: Dict available_tools = ", ".join(tool_handlers.keys()) raise ValueError(f"Tool '{tool_name}' not found in hybrid control system. Available tools: {available_tools}") - def _is_file_operation(self, action: str) -> bool: - """Check if action is a file operation.""" - # Simple check for 'file' action - if action.strip() == "file": - return True - - # More specific patterns that indicate file operations - file_patterns = [ - r"write.*to\s+(?:a\s+)?(?:file|path)", # Write ... to file/path - r"save.*to\s+(?:a\s+)?(?:file|path)", # Save ... to file/path - r"write.*following.*content.*to", # Write the following content to - r"save.*following.*content.*to", # Save the following content to - r"create.*file\s+at", # Create file at - r"export.*to\s+file", # Export to file - r"store.*in\s+file", # Store in file - r"write.*to\s+[^\s]+\.(txt|md|json|yaml|yml|csv|html)", # Write to filename.ext - r"save.*to\s+[^\s]+\.(txt|md|json|yaml|yml|csv|html)", # Save to filename.ext - ] - return any( - re.search(pattern, action, re.IGNORECASE | re.DOTALL) - for pattern in file_patterns - ) - - def _is_echo_operation(self, action: str) -> bool: - """Check if action is a simple echo/print operation.""" - echo_patterns = [ - r"^echo\s+", - r"^print\s+", - r"^display\s+", - r"^show\s+", - r"^output\s+", - ] - return any( - re.match(pattern, action, re.IGNORECASE) for pattern in echo_patterns - ) - async def _handle_echo_operation( self, task: Task, context: Dict[str, Any] ) -> Dict[str, Any]: @@ -480,7 +442,14 @@ async def _handle_file_operation( return await self.filesystem_tool.execute(**resolved_params) # If the action is a known filesystem operation, use FileSystemTool - filesystem_actions = ["read", "write", "copy", "move", "delete", "list", "file"] + # Mixes operations (read, write, ...) with names for the tool itself. + # `filesystem` is the canonical spelling of the latter and `file` its + # alias; both must appear or a step reaching this branch under its + # canonical name silently falls through to the default below. + filesystem_actions = [ + "read", "write", "copy", "move", "delete", "list", + "file", "filesystem", + ] if action_text in filesystem_actions and task.parameters: # Use UnifiedTemplateResolver for template resolution resolved_params = task.parameters.copy() diff --git a/src/orchestrator/control_systems/model_based_control_system.py b/src/orchestrator/control_systems/model_based_control_system.py index 451dda56..11e34af8 100644 --- a/src/orchestrator/control_systems/model_based_control_system.py +++ b/src/orchestrator/control_systems/model_based_control_system.py @@ -6,7 +6,12 @@ import re from typing import Any, Dict, List, Optional -from ..core.actions import STRUCTURED_ACTIONS +from ..core.actions import ( + STRUCTURED_ACTIONS, + SUPPORTED_ACTIONS, + is_known_action, +) +from ..core.exceptions import UnknownActionError from ..core.control_system import ControlSystem from ..core.pipeline import Pipeline from ..core.task import Task @@ -37,25 +42,9 @@ def __init__( if config is None: config = { "capabilities": { - "supported_actions": [ - "generate", - "generate_text", - "generate_structured", - "analyze", - "transform", - "execute", - "search", - "extract", - "filter", - "synthesize", - "create", - "validate", - "optimize", - "review", - "write", - "compile", - "process", - ], + # Generated from core/actions.py, so this can no + # longer advertise an action nobody implements. + "supported_actions": list(SUPPORTED_ACTIONS), "parallel_execution": True, "streaming": True, "checkpoint_support": True, @@ -217,9 +206,18 @@ async def _execute_task_impl(self, task: Task, context: Dict[str, Any]) -> Any: elif analysis_type == "trends": prompt += "\n\nIdentify and analyze key trends, patterns, and insights from this data." else: - # For other actions, use the action as the prompt action_text = str(task.action) # Convert to string in case it's not + # Anything reaching here is being turned into a prompt, which is + # only ever correct for an explicit ... instruction. + # It used to be the fate of *every* unrecognised action, so a typo + # like `action: gernate` became a successful model call instead of + # an error. The check is repeated here rather than left to the + # compiler because a caller can build a Task and reach dispatch + # directly, without going through YAML validation at all. + if not is_known_action(action_text): + raise UnknownActionError(action_text, task_id=task.id) + # Handle AUTO tags by extracting the content auto_tag_pattern = r"(.*?)" auto_match = re.search(auto_tag_pattern, action_text, re.DOTALL) diff --git a/src/orchestrator/core/actions.py b/src/orchestrator/core/actions.py index c31e6380..df9886a3 100644 --- a/src/orchestrator/core/actions.py +++ b/src/orchestrator/core/actions.py @@ -1,80 +1,337 @@ -"""The actions the runtime executes itself, without a `tool:`. +"""The action vocabulary: one authoritative registry, several generated views. A step names either a tool and an operation on it:: tool: filesystem action: read -or an action the runtime handles directly:: +or an action the runtime executes itself:: action: generate + parameters: + prompt: "..." -Both the executor and the validator need to know which names fall in the -second group, and they used to know separately. `ToolValidator` treated a -step's `action:` as a tool name whenever the step had no `tool:` key, so a -model step was reported as ``Tool 'generate' not found in registry`` while the -executor ran the very same step correctly -- #241, `validate` and `run` -disagreeing about the same document. - -This module is the single source of truth. `HybridControlSystem` *dispatches* -off `BUILTIN_ACTION_HANDLERS`, so the mapping cannot claim an action the -executor does not actually run, and the validator accepts exactly the names in -it. Adding a runtime action means adding one entry here; both sides follow. - -Not covered here are the natural-language action families -- ``"echo ..."``, -``"write the following content to report.md"`` -- which are matched by pattern -rather than by name. Those are `HybridControlSystem._is_echo_operation` and -`_is_file_operation`. +This module owns the second group. Everything downstream is *derived* from +`ACTION_SPECS` rather than restated beside it: the executor's dispatch table, +the validator's notion of a recognised action, the control systems' advertised +`supported_actions`, alias normalisation in the compiler, the documented +vocabulary, and the parametrised contract tests. A name is supported exactly +when there is an `ActionSpec` for it. + +The vocabulary had drifted into three disagreeing definitions -- #241 closed +the first pair, this module closes the rest: + +1. `HybridControlSystem`'s dispatch chain -- what actually ran. +2. `ModelBasedControlSystem.supported_actions` -- which advertised ten names + (`transform`, `search`, `extract`, `filter`, `synthesize`, `create`, + `optimize`, `review`, `write`, `compile`) that had no handler at all. +3. An implicit fallback turning *any* unrecognised action into a prompt, so + `action: gernate` did not fail -- it silently became a model request and + reported success. + +Number 3 is the dangerous one, and it is gone: an action with no spec is +refused, at compile time and again at dispatch. A census of 130 pipeline +documents found 430 steps dispatched by action name, of which 247 already used +a registered action and 167 relied on the prompt fallback -- including +`generate-text`, a hyphen slip of `generate_text` that had never dispatched. + +Two families remain matched by pattern rather than by name, because prose has +no name to register: ``"echo ..."`` and ``"write the following content to +report.md"``. They are declared as `ActionFamily` entries so they belong to +this vocabulary rather than being another undocumented special case. """ from __future__ import annotations -from typing import Dict, FrozenSet +import re +from dataclasses import dataclass +from typing import Any, Dict, FrozenSet, Optional, Pattern, Tuple -# Action name -> the `HybridControlSystem` method that runs it. +# --- what a step's result must look like ----------------------------------- # -# Values are method *names* rather than functions because the handlers are -# bound methods on the control system; the dispatcher resolves them per call. -BUILTIN_ACTION_HANDLERS: Dict[str, str] = { - "control_flow": "_handle_control_flow", - # `file` and `filesystem` are the two spellings the executor has always - # accepted for the filesystem handler. - "file": "_handle_file_operation", - "filesystem": "_handle_file_operation", - "process": "_handle_data_processing", - "validate": "_handle_validation", - "loop_complete": "_handle_loop_complete", - "capture_result": "_handle_capture_result", - "evaluate_condition": "_handle_evaluate_condition", - "create_parallel_queue": "_handle_create_parallel_queue", - "action_loop": "_handle_action_loop", - "analyze": "_handle_analyze_text", - "analyze_text": "_handle_analyze_text", - "generate": "_handle_generate_text", - "generate_text": "_handle_generate_text", +# Handlers return a mapping carrying the outcome. These are the envelopes the +# contract tests assert against, so a handler cannot quietly change shape. + +_TEXT_RESULT_SCHEMA: Dict[str, Any] = { + "type": "object", + "required": ["success", "result"], + "properties": { + "success": {"type": "boolean"}, + "result": {"type": "string"}, + "action": {"type": "string"}, + "model_used": {"type": "string"}, + "error": {"type": ["string", "null"]}, + }, } -# Structured generation is handled one level down, by -# `ModelBasedControlSystem`, so it has no entry above -- but it is still an -# action the runtime executes without a tool, and the validator must accept it. +_OBJECT_RESULT_SCHEMA: Dict[str, Any] = {"type": "object"} + + +@dataclass(frozen=True) +class ActionSpec: + """One action the runtime executes without a `tool:`.""" + + #: The single spelling that appears in the task graph and the trace. + name: str + + #: One-line description, used to generate the documented vocabulary. + summary: str + + #: Method on `HybridControlSystem` that runs it. `None` means the action is + #: handled further down, by `ModelBasedControlSystem`. + handler: Optional[str] = None + + #: Accepted spellings that normalise to `name`. Kept for compatibility; + #: the compiler rewrites them and warns. + aliases: FrozenSet[str] = frozenset() + + #: "model", "tool", or "none" -- what the action needs to do its work. + requires: str = "none" + + #: Parameters without which the action cannot run. Checked by the + #: validator, so a missing one fails at compile time rather than at 3am. + required_parameters: FrozenSet[str] = frozenset() + + #: JSON Schema the step's result must satisfy, where the shape is fixed. + result_schema: Optional[Dict[str, Any]] = None + + #: Whether this is part of the supported v1 contract. Everything shipped + #: today is; the flag exists so adding an experimental action later does + #: not require inventing a second registry for it. + v1: bool = True + + def __post_init__(self) -> None: + if self.requires not in {"model", "tool", "none"}: + raise ValueError( + f"action {self.name!r}: requires must be 'model', 'tool' or " + f"'none', got {self.requires!r}" + ) + if self.name in self.aliases: + raise ValueError(f"action {self.name!r} lists its own name as an alias") + + +@dataclass(frozen=True) +class ActionFamily: + """A family of actions matched by pattern, because they are prose. + + ``"echo Starting run"`` and ``"write the following content to report.md"`` + are instructions, not identifiers. They still dispatch deterministically -- + they simply cannot be looked up by name. + """ + + name: str + summary: str + #: Method on `HybridControlSystem`, or `None` for "hand it to the model". + handler: Optional[str] + pattern: Pattern[str] + v1: bool = True + + +ACTION_SPECS: Tuple[ActionSpec, ...] = ( + # -- model actions ------------------------------------------------------ + ActionSpec( + name="generate", + summary="Generate text from a prompt using the selected model.", + handler="_handle_generate_text", + aliases=frozenset({"generate_text", "generate-text"}), + requires="model", + required_parameters=frozenset({"prompt"}), + result_schema=_TEXT_RESULT_SCHEMA, + ), + ActionSpec( + name="generate_structured", + summary="Generate an object conforming to a declared JSON Schema.", + # No handler entry: ModelBasedControlSystem runs this one. + aliases=frozenset({"generate-structured"}), + requires="model", + required_parameters=frozenset({"prompt", "schema"}), + result_schema=_OBJECT_RESULT_SCHEMA, + ), + ActionSpec( + name="analyze_text", + summary="Analyse supplied text with the selected model.", + handler="_handle_analyze_text", + aliases=frozenset({"analyze"}), + requires="model", + ), + # -- deterministic runtime actions -------------------------------------- + ActionSpec( + name="filesystem", + summary="Filesystem operation named by the step's `action` parameter.", + handler="_handle_file_operation", + aliases=frozenset({"file"}), + requires="tool", + ), + ActionSpec( + name="process", + summary="Transform data with the deterministic data-processing tool.", + handler="_handle_data_processing", + requires="tool", + ), + ActionSpec( + name="validate", + summary="Validate data against a schema with the validation tool.", + handler="_handle_validation", + requires="tool", + ), + # -- control flow ------------------------------------------------------- + ActionSpec( + name="control_flow", + summary="Control-flow marker step.", + handler="_handle_control_flow", + ), + ActionSpec( + name="evaluate_condition", + summary="Evaluate a condition expression and record the verdict.", + handler="_handle_evaluate_condition", + required_parameters=frozenset({"condition"}), + ), + ActionSpec( + name="loop_complete", + summary="Marker emitted when a loop finishes.", + handler="_handle_loop_complete", + ), + ActionSpec( + name="capture_result", + summary="Capture a prior step's result under a new name.", + handler="_handle_capture_result", + ), + ActionSpec( + name="create_parallel_queue", + summary="Fan work out across a parallel queue.", + handler="_handle_create_parallel_queue", + ), + ActionSpec( + name="action_loop", + summary="Run a sequence of actions repeatedly until a condition holds.", + handler="_handle_action_loop", + ), +) + + +ACTION_FAMILIES: Tuple[ActionFamily, ...] = ( + ActionFamily( + name="echo", + summary='Print a message, e.g. `action: echo "starting"`.', + handler="_handle_echo_operation", + pattern=re.compile(r"^(?:echo|print|display|show|output)\s+", re.IGNORECASE), + ), + ActionFamily( + name="file_prose", + summary=( + "Filesystem instruction written as prose, e.g. " + "`action: write the following content to report.md`." + ), + handler="_handle_file_operation", + pattern=re.compile( + r"write.*to\s+(?:a\s+)?(?:file|path)" + r"|save.*to\s+(?:a\s+)?(?:file|path)" + r"|write.*following.*content.*to" + r"|save.*following.*content.*to" + r"|create.*file\s+at" + r"|export.*to\s+file" + r"|store.*in\s+file" + r"|write.*to\s+[^\s]+\.(?:txt|md|json|yaml|yml|csv|html)" + r"|save.*to\s+[^\s]+\.(?:txt|md|json|yaml|yml|csv|html)", + re.IGNORECASE | re.DOTALL, + ), + ), + ActionFamily( + name="auto", + summary=( + "An explicit request for the model to interpret the instruction, " + "e.g. `action: summarise the findings`." + ), + # No handler: this is the one case that is *meant* to reach the model. + handler=None, + pattern=re.compile(r".*?", re.IGNORECASE | re.DOTALL), + ), +) + + +def _build_index() -> Dict[str, ActionSpec]: + index: Dict[str, ActionSpec] = {} + for spec in ACTION_SPECS: + for spelling in (spec.name, *spec.aliases): + key = spelling.lower() + if key in index: + raise ValueError( + f"action spelling {spelling!r} is claimed by both " + f"{index[key].name!r} and {spec.name!r}" + ) + index[key] = spec + return index + + +_BY_SPELLING: Dict[str, ActionSpec] = _build_index() + + +# --- generated views ------------------------------------------------------- # -# Every other action here uses underscores. `generate-structured` was the lone -# hyphenated one, and for a long time the only spelling that dispatched: the -# underscore fell through to the natural-language branch, which turns an -# unrecognised action into a *prompt*, so the step returned a sentence instead -# of an object and still reported success. Both spellings are supported. +# Every one of these is derived. None is maintained by hand, so none can drift. + +#: Action spelling -> the `HybridControlSystem` method that runs it. Values are +#: method *names* because the handlers are bound methods on the control system; +#: the dispatcher resolves them per call. +BUILTIN_ACTION_HANDLERS: Dict[str, str] = { + spelling: spec.handler + for spelling, spec in _BY_SPELLING.items() + if spec.handler is not None +} + +#: Every spelling the runtime executes without a `tool:`, aliases included. +BUILTIN_ACTIONS: FrozenSet[str] = frozenset(_BY_SPELLING) + +#: The spellings of the structured-generation action. STRUCTURED_ACTIONS: FrozenSet[str] = frozenset( - {"generate-structured", "generate_structured"} + spelling + for spelling, spec in _BY_SPELLING.items() + if spec.name == "generate_structured" +) + +#: What the control systems advertise. Generated, so it can no longer promise +#: an action nobody implements. +SUPPORTED_ACTIONS: Tuple[str, ...] = tuple( + sorted(spelling for spelling, spec in _BY_SPELLING.items() if spec.v1) ) -#: Every action name the runtime executes without a `tool:`. -BUILTIN_ACTIONS: FrozenSet[str] = frozenset(BUILTIN_ACTION_HANDLERS) | STRUCTURED_ACTIONS + +def resolve_action(action: str) -> Optional[ActionSpec]: + """The spec for `action`, by canonical name or alias, else `None`. + + Matching is case-insensitive and ignores surrounding whitespace, because + the executor lowercases a task's action before dispatching on it. + """ + if not isinstance(action, str): + return None + return _BY_SPELLING.get(action.strip().lower()) + + +def canonical_action(action: str) -> Optional[str]: + """The one spelling that should appear in the task graph and the trace.""" + spec = resolve_action(action) + return spec.name if spec else None def is_builtin_action(action: str) -> bool: - """Whether `action` names something the runtime executes without a tool. + """Whether `action` names something the runtime executes without a tool.""" + return resolve_action(action) is not None + + +def match_action_family(action: str) -> Optional[ActionFamily]: + """The prose family `action` belongs to, if any.""" + if not isinstance(action, str): + return None + for family in ACTION_FAMILIES: + if family.pattern.search(action): + return family + return None + + +def is_known_action(action: str) -> bool: + """Whether anything at all can execute `action`. - Comparison is case-insensitive and ignores surrounding whitespace, matching - the executor, which lowercases a task's action before dispatching on it. + The question both the validator and the dispatcher ask. Anything that + answers `False` is refused rather than turned into a model prompt. """ - return action.strip().lower() in BUILTIN_ACTIONS + return is_builtin_action(action) or match_action_family(action) is not None diff --git a/src/orchestrator/core/exceptions.py b/src/orchestrator/core/exceptions.py index c271be94..d5d9056c 100644 --- a/src/orchestrator/core/exceptions.py +++ b/src/orchestrator/core/exceptions.py @@ -205,6 +205,30 @@ def __init__(self, parameter: str, reason: str, **kwargs): ) +class UnknownActionError(ValidationError): + """Raised when a step names an action nothing can execute. + + The runtime used to turn any unrecognised action into a prompt for the + model, so `action: gernate` did not fail -- it became a model request and + reported success. A typo is now refused: at compile time by the validator, + and again at dispatch, so constructing a `Task` directly cannot route + around the check. + """ + + def __init__(self, action: str, task_id: Optional[str] = None, **kwargs): + where = f" in task '{task_id}'" if task_id else "" + message = ( + f"Unknown action {action!r}{where}. An action must name a " + f"registered action (see orchestrator.core.actions), name a tool " + f"via `tool:`, or be an explicit ... instruction." + ) + super().__init__( + message, + details={"action": action, "task_id": task_id}, + **kwargs + ) + + class UnresolvedTemplateError(ValidationError): """Raised when a template reaches a tool with references still unresolved. diff --git a/src/orchestrator/orchestrator.py b/src/orchestrator/orchestrator.py index df4b7bac..6c0f24c2 100644 --- a/src/orchestrator/orchestrator.py +++ b/src/orchestrator/orchestrator.py @@ -1610,8 +1610,20 @@ async def _execute_level( except Exception: # Fallback to original error handled_error = result - task.fail(handled_error) - results[task_id] = {"error": str(handled_error)} + # `handle_error` returns a *recovery decision* -- + # {"action": "retry", "delay": 5.0, ...} -- not an + # error. Recording that as the task's error meant every + # failed step reported a retry policy instead of the + # reason it failed, and `task.error` was not even an + # exception. The reason is the original exception; the + # decision is kept beside it rather than on top of it. + task.fail(result) + results[task_id] = { + "success": False, + "error": str(result), + "error_type": type(result).__name__, + "recovery": handled_error, + } else: # Task succeeded results[task_id] = result @@ -2039,8 +2051,21 @@ async def _handle_task_failures( # Skip dependent tasks self._skip_dependent_tasks(pipeline, task_id) elif failure_policy == "fail": - # Fail entire pipeline - raise ExecutionError(f"Task '{task_id}' failed and policy is 'fail'") + # Fail entire pipeline. + # + # The task already knows why it failed; reporting only "failed + # and policy is 'fail'" threw that away and left the caller to + # go hunting in the logs. The reason is carried in the message + # and chained, so `raise ... from` preserves the original + # traceback and type for anyone catching it. + # `Task.error` is typed Optional[Exception] but is not always + # populated with one, so chaining is conditional. + cause = task.error + reason = f": {cause}" if cause else "" + message = f"Task '{task_id}' failed and policy is 'fail'{reason}" + if isinstance(cause, BaseException): + raise ExecutionError(message) from cause + raise ExecutionError(message) elif failure_policy == "retry": # Retry is handled in _execute_task continue diff --git a/src/orchestrator/validation/tool_validator.py b/src/orchestrator/validation/tool_validator.py index 9cb335d7..9f6f2a43 100644 --- a/src/orchestrator/validation/tool_validator.py +++ b/src/orchestrator/validation/tool_validator.py @@ -4,7 +4,7 @@ from typing import Any, Dict, List, Optional, Set, Type, Union from dataclasses import dataclass -from ..core.actions import is_builtin_action +from ..core.actions import is_known_action, resolve_action from ..tools.base import Tool, ToolRegistry, default_registry from .template_validator import TemplateValidationError @@ -151,7 +151,10 @@ def validate_pipeline_tools(self, pipeline_def: Dict[str, Any]) -> ToolValidatio # The tool lookup still comes first, so the legacy single-field # form (`action: filesystem`, naming the tool itself) keeps its # full parameter validation. - if not tool_available and "tool" not in step and is_builtin_action(tool_name): + if not tool_available and "tool" not in step and is_known_action(tool_name): + errors.extend( + self._validate_action_parameters(task_id, step, tool_name) + ) continue tool_availability[tool_name] = tool_available @@ -204,6 +207,35 @@ def validate_pipeline_tools(self, pipeline_def: Dict[str, Any]) -> ToolValidatio tool_availability=tool_availability ) + def _validate_action_parameters( + self, task_id: str, step: Dict[str, Any], action: str + ) -> List[ToolValidationError]: + """Check the parameters an action cannot run without. + + `ActionSpec.required_parameters` is the single declaration of these, so + a missing `prompt` on a `generate` step fails at compile time with exit + 2 rather than at dispatch. Prose families carry no spec and no required + parameters, so they are not checked here. + """ + spec = resolve_action(action) + if spec is None or not spec.required_parameters: + return [] + + parameters = step.get("parameters") or {} + missing = sorted(spec.required_parameters - set(parameters)) + return [ + ToolValidationError( + task_id=task_id, + tool_name=spec.name, + parameter_name=name, + error_type="missing_parameter", + message=( + f"action '{spec.name}' requires a '{name}' parameter" + ), + ) + for name in missing + ] + def _extract_tool_name(self, step: Dict[str, Any]) -> Optional[str]: """Extract tool name from step definition.""" # When a step names a `tool`, that is the registry key and `action` is diff --git a/tests/test_action_contract.py b/tests/test_action_contract.py new file mode 100644 index 00000000..c8a24186 --- /dev/null +++ b/tests/test_action_contract.py @@ -0,0 +1,413 @@ +"""The action vocabulary contract. + +`core/actions.py` is the one authoritative registry. Everything else -- the +executor's dispatch, the validator's recognition, the control systems' +advertised capabilities, alias normalisation, the documented vocabulary -- is +derived from it. These tests pin that derivation in both directions, so the +vocabulary cannot drift back apart. + +The behaviour being protected, concretely: `action: gernate` must fail. It used +to become a successful model call, because any unrecognised action was turned +into a prompt. +""" + +import asyncio +import warnings + +import jsonschema +import pytest + +from orchestrator.core.actions import ( + ACTION_FAMILIES, + ACTION_SPECS, + BUILTIN_ACTION_HANDLERS, + SUPPORTED_ACTIONS, + ActionSpec, + canonical_action, + is_known_action, + match_action_family, + resolve_action, +) +from orchestrator.compiler.yaml_compiler import YAMLCompiler +from orchestrator.control_systems.hybrid_control_system import HybridControlSystem +from orchestrator.core.exceptions import UnknownActionError, YAMLCompilerError +from orchestrator.core.task import Task +from orchestrator.models.model_registry import ModelRegistry +from orchestrator.validation.tool_validator import ToolValidator +from tests.test_infrastructure import MockTestModel, create_test_orchestrator + +pytestmark = [pytest.mark.contract] + + +@pytest.fixture +def control_system(): + registry = ModelRegistry() + registry.register_model(MockTestModel()) + return HybridControlSystem(model_registry=registry) + + +@pytest.fixture +def validator(): + """Strict: unknown names are errors, not warnings.""" + return ToolValidator(allow_unknown_tools=False) + + +@pytest.fixture +def compiler(): + registry = ModelRegistry() + registry.register_model(MockTestModel()) + return YAMLCompiler(model_registry=registry) + + +def _pipeline(action, parameters="{}"): + return f""" +id: action_contract_pipeline +name: Action Contract Pipeline +steps: + - id: only_step + action: {action} + parameters: {parameters} +""" + + +# --------------------------------------------------------------------------- +# 1. every registered action validates and dispatches +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("spec", ACTION_SPECS, ids=lambda s: s.name) +def test_every_registered_action_is_recognised_everywhere(spec, validator): + """Registry, validator and canonical lookup must agree on every spelling.""" + for spelling in (spec.name, *spec.aliases): + assert is_known_action(spelling), f"{spelling!r} unknown to the registry" + assert canonical_action(spelling) == spec.name + + result = validator.validate_pipeline_tools( + {"steps": [{"id": "s", "action": spelling, + "parameters": {p: "x" for p in spec.required_parameters}}]} + ) + rejected = [e for e in result.errors if e.error_type == "unknown_tool"] + assert not rejected, f"{spelling!r} rejected as a tool: {rejected}" + + +@pytest.mark.parametrize( + "spec", [s for s in ACTION_SPECS if s.handler], ids=lambda s: s.name +) +def test_every_registered_handler_exists(spec, control_system): + """Dispatch is `getattr(self, handler)`, so a bad name is an AttributeError. + + Naming the offending action here beats discovering it inside whichever + pipeline happened to use it. + """ + handler = getattr(control_system, spec.handler, None) + assert handler is not None, f"{spec.name!r} -> missing {spec.handler!r}" + assert callable(handler) + + +@pytest.mark.parametrize("family", ACTION_FAMILIES, ids=lambda f: f.name) +def test_every_family_handler_exists(family, control_system): + """`auto` deliberately carries no handler -- it is the model path.""" + if family.handler is None: + assert family.name == "auto" + return + assert callable(getattr(control_system, family.handler, None)) + + +def test_no_handler_is_registered_without_a_spec(control_system): + """The reverse direction: dispatch cannot reach an unregistered method.""" + for spelling, handler_name in BUILTIN_ACTION_HANDLERS.items(): + assert resolve_action(spelling) is not None + assert hasattr(control_system, handler_name) + + +def test_supported_actions_is_generated_not_restated(control_system): + """The advertised capability list must be the registry, not a copy of it. + + It used to advertise ten names -- transform, search, extract, filter, + synthesize, create, optimize, review, write, compile -- that had no handler + anywhere. Anything it lists must now be executable. + """ + advertised = control_system._capabilities.get("supported_actions", []) + + assert set(advertised) == set(SUPPORTED_ACTIONS) + for name in advertised: + assert is_known_action(name), f"advertised but unknown: {name!r}" + + +# --------------------------------------------------------------------------- +# 2. aliases normalise identically +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "alias,canonical", + [(a, s.name) for s in ACTION_SPECS for a in sorted(s.aliases)], +) +def test_alias_normalises_in_the_compiled_task_graph(alias, canonical, compiler): + """One spelling reaches the task graph, whichever the author wrote.""" + parameters = "{" + ", ".join( + f"{p}: x" for p in sorted(resolve_action(canonical).required_parameters) + ) + "}" + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + pipeline = asyncio.run( + compiler.compile(_pipeline(alias, parameters), {}, resolve_ambiguities=False) + ) + + assert pipeline.tasks["only_step"].action == canonical, ( + f"alias {alias!r} was left unnormalised in the task graph" + ) + + +def test_a_deprecated_alias_warns(compiler): + """Silent rewriting would leave authors with no reason to update.""" + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + asyncio.run( + compiler.compile( + _pipeline("generate-structured", '{prompt: p, schema: {type: object}}'), + {}, + resolve_ambiguities=False, + ) + ) + + messages = [str(w.message) for w in caught if issubclass(w.category, DeprecationWarning)] + assert any("generate-structured" in m and "generate_structured" in m for m in messages), ( + f"no deprecation warning naming both spellings: {messages}" + ) + + +# --------------------------------------------------------------------------- +# 3. unknown actions fail, through YAML *and* through direct execution +# --------------------------------------------------------------------------- + +def test_an_unknown_action_fails_to_compile(compiler): + """`gernate` is a typo, not an instruction.""" + with pytest.raises(YAMLCompilerError) as excinfo: + asyncio.run( + compiler.compile( + _pipeline("gernate", "{prompt: hello}"), {}, resolve_ambiguities=False + ) + ) + + assert "gernate" in str(excinfo.value) + + +def test_an_unknown_action_fails_at_dispatch_too(control_system): + """Compile-time validation must not be the only safety boundary. + + A caller can build a Task and reach dispatch without going through YAML at + all, so the runtime refuses independently. + """ + task = Task(id="typo", name="typo", action="gernate", parameters={"prompt": "hi"}) + + with pytest.raises(UnknownActionError) as excinfo: + asyncio.run(control_system._execute_task_impl(task, {})) + + assert excinfo.value.details["action"] == "gernate" + assert excinfo.value.details["task_id"] == "typo" + + +def test_a_typo_never_becomes_a_successful_model_call(): + """The regression this whole contract exists to prevent. + + Before: `action: gernate` was turned into a prompt, the model answered, and + the step reported success. A misspelling produced a plausible-looking + result instead of an error. + """ + # `on_failure: continue` so the run completes and the step's own envelope + # can be inspected. Under the default policy the pipeline raises instead, + # which the assertion below also covers. + tolerant = """ +id: typo_pipeline +name: Typo Pipeline +steps: + - id: only_step + action: gernate + on_failure: continue + parameters: + prompt: hello +""" + orchestrator = create_test_orchestrator() + result = asyncio.run(orchestrator.execute_yaml(yaml_content=tolerant, context={})) + + step = result["only_step"] + assert "error" in step, f"a typo must not report success: {step}" + assert "gernate" in str(step["error"]), ( + f"the step's error must name the offending action: {step['error']}" + ) + + # And with the default policy, the run fails outright rather than + # returning a plausible-looking model answer. + with pytest.raises(Exception): + asyncio.run( + orchestrator.execute_yaml( + yaml_content=_pipeline("gernate", "{prompt: hello}"), context={} + ) + ) + + +def test_an_explicit_auto_instruction_is_still_allowed(control_system): + """Removing the implicit fallback must not remove the explicit feature. + + `...` is an author deliberately asking the model to interpret + an instruction. That is not the same as a typo falling through. + """ + assert is_known_action("summarise the findings") + assert match_action_family("summarise the findings").name == "auto" + + +@pytest.mark.parametrize("prose", ['echo "starting"', "write the following content to report.md"]) +def test_declared_prose_families_are_still_allowed(prose): + """Prose families are declared, not arbitrary. They keep working.""" + assert is_known_action(prose) + assert match_action_family(prose) is not None + + +# --------------------------------------------------------------------------- +# 4. required parameters are checked at compile time +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "spec", [s for s in ACTION_SPECS if s.required_parameters], ids=lambda s: s.name +) +def test_missing_required_parameter_is_reported(spec, validator): + """Exit 2, not a mystery at dispatch.""" + result = validator.validate_pipeline_tools( + {"steps": [{"id": "s", "action": spec.name, "parameters": {}}]} + ) + + missing = [e for e in result.errors if e.error_type == "missing_parameter"] + assert {e.parameter_name for e in missing} == set(spec.required_parameters), ( + f"expected every one of {sorted(spec.required_parameters)} to be reported, " + f"got {[e.parameter_name for e in missing]}" + ) + + +def test_supplying_required_parameters_validates_cleanly(validator): + result = validator.validate_pipeline_tools( + {"steps": [{"id": "s", "action": "generate", "parameters": {"prompt": "hi"}}]} + ) + + assert not [e for e in result.errors if e.error_type == "missing_parameter"] + + +# --------------------------------------------------------------------------- +# 5. result envelopes match their declared schemas +# --------------------------------------------------------------------------- + +def test_generate_result_matches_its_declared_schema(): + """The spec's result_schema is a promise, so it is checked against a run.""" + spec = resolve_action("generate") + orchestrator = create_test_orchestrator() + + result = asyncio.run( + orchestrator.execute_yaml( + yaml_content=_pipeline("generate", "{prompt: summarise this}"), context={} + ) + ) + + jsonschema.validate(instance=result["only_step"], schema=spec.result_schema) + + +def test_structured_result_matches_its_declared_schema(): + spec = resolve_action("generate_structured") + orchestrator = create_test_orchestrator() + + result = asyncio.run( + orchestrator.execute_yaml( + yaml_content=_pipeline( + "generate_structured", + '{prompt: extract, schema: {type: object, properties: {test_output: {type: string}}}}', + ), + context={}, + ) + ) + + jsonschema.validate(instance=result["only_step"], schema=spec.result_schema) + + +# --------------------------------------------------------------------------- +# 6. the registry itself is well formed +# --------------------------------------------------------------------------- + +def test_no_spelling_is_claimed_twice(): + """Enforced at import; asserted here so the reason is recorded.""" + seen = set() + for spec in ACTION_SPECS: + for spelling in (spec.name, *spec.aliases): + assert spelling.lower() not in seen, f"duplicate spelling {spelling!r}" + seen.add(spelling.lower()) + + +def test_requires_is_constrained(): + with pytest.raises(ValueError, match="requires must be"): + ActionSpec(name="bogus", summary="x", requires="magic") + + +def test_a_spec_cannot_alias_itself(): + with pytest.raises(ValueError, match="its own name as an alias"): + ActionSpec(name="bogus", summary="x", aliases=frozenset({"bogus"})) + + +def test_every_spec_has_a_summary_for_the_generated_docs(): + for spec in ACTION_SPECS: + assert spec.summary.strip(), f"{spec.name} has no summary" + for family in ACTION_FAMILIES: + assert family.summary.strip(), f"{family.name} has no summary" + + +# --------------------------------------------------------------------------- +# 7. the tool forms are unaffected by the action contract +# +# Folded in from tests/test_builtin_actions.py (#241), whose remaining +# assertions duplicated the ones above once the registry became authoritative. +# --------------------------------------------------------------------------- + +def test_the_legacy_single_field_form_still_resolves_as_a_tool(validator): + """`action: filesystem` names the tool itself and keeps full validation. + + `filesystem` is both a registered tool and a registered action name. The + tool lookup has to win, or this form silently loses its parameter checks. + """ + result = validator.validate_pipeline_tools( + {"steps": [{"id": "s", "action": "filesystem", "parameters": {}}]} + ) + + assert result.tool_availability.get("filesystem") is True + + +def test_the_two_field_form_resolves_tool_never_action(validator): + result = validator.validate_pipeline_tools( + { + "steps": [ + { + "id": "s", + "tool": "filesystem", + "action": "read", + "parameters": {"path": "x.txt"}, + } + ] + } + ) + + assert "filesystem" in result.tool_availability + assert "read" not in result.tool_availability, ( + "`read` is an operation on the tool, not a tool to look up" + ) + + +def test_a_builtin_action_dispatches_away_from_the_model(control_system): + """`evaluate_condition` is a marker action with no model involvement. + + Reaching the model would produce prose instead of a verdict, which is the + boundary the whole contract exists to hold. + """ + task = Task( + id="check", + name="check", + action="evaluate_condition", + parameters={"condition": "1 == 1"}, + ) + result = asyncio.run(control_system._execute_task_impl(task, {})) + + assert isinstance(result, dict) + assert "result" in result or "condition" in result diff --git a/tests/test_action_contract_docs.py b/tests/test_action_contract_docs.py new file mode 100644 index 00000000..05aa387b --- /dev/null +++ b/tests/test_action_contract_docs.py @@ -0,0 +1,62 @@ +"""The documented action vocabulary must match the registry. + +`docs/actions.md` is generated from `orchestrator.core.actions`. Generating it +is only useful if regeneration is enforced -- otherwise the committed file +drifts and the documentation quietly starts describing a vocabulary the code no +longer has, which is how this project ended up advertising ten actions with no +handler in the first place. +""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.contract] + +ROOT = Path(__file__).resolve().parent.parent +GENERATOR = ROOT / "scripts" / "generate_action_docs.py" +DOC = ROOT / "docs" / "actions.md" + + +def test_the_generated_action_docs_are_committed_and_current(): + result = subprocess.run( + [sys.executable, str(GENERATOR), "--check"], + cwd=str(ROOT), + capture_output=True, + text=True, + timeout=120, + ) + + assert result.returncode == 0, ( + f"docs/actions.md is out of date. Run:\n" + f" python scripts/generate_action_docs.py\n\n" + f"{result.stdout}\n{result.stderr}" + ) + + +def test_every_registered_action_appears_in_the_documentation(): + """Belt and braces: the check above compares bytes, this checks intent.""" + from orchestrator.core.actions import ACTION_FAMILIES, ACTION_SPECS + + text = DOC.read_text() + for spec in ACTION_SPECS: + assert f"`{spec.name}`" in text, f"{spec.name} is undocumented" + for alias in spec.aliases: + assert f"`{alias}`" in text, f"alias {alias} is undocumented" + for family in ACTION_FAMILIES: + assert f"`{family.name}`" in text, f"family {family.name} is undocumented" + + +def test_the_exit_code_boundary_is_documented(): + """#153's runtime boundary versus #241's compile-time one. + + Both are failures, they are not the same failure, and the difference is + visible to anyone scripting the CLI. + """ + text = DOC.read_text() + + assert "**2**" in text and "**1**" in text + assert "compile time" in text + assert "before any side effect" in text diff --git a/tests/test_builtin_actions.py b/tests/test_builtin_actions.py deleted file mode 100644 index 70f63ce7..00000000 --- a/tests/test_builtin_actions.py +++ /dev/null @@ -1,176 +0,0 @@ -"""The built-in action registry, and the two consumers that must agree on it. - -`validate` and `run` used to hold separate notions of what an action is. The -validator treated a step's `action:` as a tool name whenever the step had no -`tool:` key, so `action: generate` was rejected as a missing *tool* while the -executor ran the same step correctly (#241). - -`core.actions` is now the single source of truth. These tests pin both -directions of that: the executor dispatches every name in the registry to a -handler that exists, and the validator accepts every name in the registry. -""" - -import asyncio - -import pytest - -from orchestrator.core.actions import ( - BUILTIN_ACTION_HANDLERS, - BUILTIN_ACTIONS, - STRUCTURED_ACTIONS, - is_builtin_action, -) -from orchestrator.control_systems.hybrid_control_system import HybridControlSystem -from orchestrator.models.model_registry import ModelRegistry -from orchestrator.validation.tool_validator import ToolValidator -from tests.test_infrastructure import MockTestModel - -pytestmark = [pytest.mark.contract] - - -@pytest.fixture -def control_system(): - registry = ModelRegistry() - registry.register_model(MockTestModel()) - return HybridControlSystem(model_registry=registry) - - -@pytest.fixture -def validator(): - """A strict validator: unknown tools are errors, not warnings.""" - return ToolValidator(allow_unknown_tools=False) - - -# --------------------------------------------------------------------------- -# the registry cannot promise what the executor cannot deliver -# --------------------------------------------------------------------------- - -@pytest.mark.parametrize("action", sorted(BUILTIN_ACTION_HANDLERS)) -def test_every_registered_action_has_a_real_handler(action, control_system): - """Dispatch is driven off this mapping, so a bad entry is an AttributeError. - - Catching it here names the offending action instead of surfacing as a - mystery failure inside whichever pipeline happened to use it. - """ - handler_name = BUILTIN_ACTION_HANDLERS[action] - handler = getattr(control_system, handler_name, None) - - assert handler is not None, ( - f"action {action!r} is registered to {handler_name!r}, " - f"which does not exist on HybridControlSystem" - ) - assert callable(handler), f"{handler_name!r} is not callable" - - -def test_structured_actions_are_builtin_but_handled_one_level_down(): - """They have no handler entry: ModelBasedControlSystem runs them. - - They must still be in BUILTIN_ACTIONS or the validator would reject them - as unknown tools -- the original #241 failure, in a different action. - """ - assert STRUCTURED_ACTIONS <= BUILTIN_ACTIONS - assert not (STRUCTURED_ACTIONS & set(BUILTIN_ACTION_HANDLERS)) - - -def test_is_builtin_action_matches_the_executors_normalisation(): - """The executor lowercases and strips before dispatching; so must this.""" - assert is_builtin_action("generate") - assert is_builtin_action(" GENERATE ") - assert is_builtin_action("Generate_Text") - assert not is_builtin_action("summarise the report in three sentences") - assert not is_builtin_action("") - - -# --------------------------------------------------------------------------- -# the validator accepts exactly what the executor runs -# --------------------------------------------------------------------------- - -@pytest.mark.parametrize("action", sorted(BUILTIN_ACTIONS)) -def test_validator_accepts_every_builtin_action(action, validator): - """No built-in action may be reported as a missing tool.""" - result = validator.validate_pipeline_tools( - {"steps": [{"id": "step_one", "action": action, "parameters": {}}]} - ) - - unknown = [e for e in result.errors if e.error_type == "unknown_tool"] - assert not unknown, ( - f"built-in action {action!r} was rejected as a tool: " - f"{[e.message for e in unknown]}" - ) - - -def test_validator_still_rejects_an_action_that_is_neither_tool_nor_builtin(validator): - """The fix must not turn the validator into a rubber stamp.""" - result = validator.validate_pipeline_tools( - {"steps": [{"id": "step_one", "action": "definitely_not_a_real_thing"}]} - ) - - assert [e for e in result.errors if e.error_type == "unknown_tool"], ( - "an unknown action still has to be reported" - ) - - -def test_validator_still_validates_the_legacy_single_field_form(validator): - """`action: filesystem` names the tool itself and keeps full validation. - - `filesystem` is both a registered tool and a built-in action name. The tool - lookup has to win, or this form would silently lose its parameter checks. - """ - result = validator.validate_pipeline_tools( - {"steps": [{"id": "step_one", "action": "filesystem", "parameters": {}}]} - ) - - assert "filesystem" in result.tool_availability, ( - "the legacy form must still be resolved as a tool, not skipped as an action" - ) - assert result.tool_availability["filesystem"] is True - - -def test_a_tool_step_is_unaffected(validator): - """The two-field form still resolves `tool:`, never `action:`.""" - result = validator.validate_pipeline_tools( - { - "steps": [ - { - "id": "step_one", - "tool": "filesystem", - "action": "read", - "parameters": {"path": "x.txt"}, - } - ] - } - ) - - assert "filesystem" in result.tool_availability - assert "read" not in result.tool_availability, ( - "`read` is an operation on the tool, not a tool to look up" - ) - - -# --------------------------------------------------------------------------- -# end to end: the two agree on the same document -# --------------------------------------------------------------------------- - -def test_builtin_action_dispatches_away_from_the_model(control_system): - """A built-in action must reach its handler, not the prompt fallback. - - The executor's last resort turns an unrecognised action into natural - language for the model. That is how `generate_structured` returned a - sentence instead of an object for as long as it did, so the boundary is - pinned: `evaluate_condition` is a marker action with no model involvement, - and reaching the model would produce prose instead of a verdict. - """ - from orchestrator.core.task import Task - - task = Task( - id="check", - name="check", - action="evaluate_condition", - parameters={"condition": "1 == 1"}, - ) - result = asyncio.run(control_system._execute_task_impl(task, {})) - - assert isinstance(result, dict), f"expected the handler's verdict, got {type(result)}" - assert "result" in result or "condition" in result, ( - f"this does not look like _handle_evaluate_condition output: {result}" - )