diff --git a/src/orchestrator/__init__.py b/src/orchestrator/__init__.py index bcca685..4d13fc1 100644 --- a/src/orchestrator/__init__.py +++ b/src/orchestrator/__init__.py @@ -89,6 +89,11 @@ "create_pipeline_api": ".api", "create_advanced_pipeline_compiler": ".api", "create_pipeline_executor": ".api", + # --- Validation, with its findings --- + "validate_pipeline_file": ".validation.pipeline_report", + "validate_pipeline_text": ".validation.pipeline_report", + "PipelineValidation": ".validation.pipeline_report", + "Finding": ".validation.pipeline_report", # --- Error hierarchy --- "OrchestratorError": ".core.exceptions", "PipelineError": ".core.exceptions", diff --git a/src/orchestrator/cli.py b/src/orchestrator/cli.py index cc4ad2c..d155d56 100644 --- a/src/orchestrator/cli.py +++ b/src/orchestrator/cli.py @@ -367,46 +367,6 @@ def _failed_steps(results) -> list: return failed -def _reportable_issues(compiler): - """Findings a successful validation should still tell the user about. - - The compiler has always collected these; nothing displayed them. A - pipeline could report `✓ is valid` while carrying a warning saying a - reference could not be checked -- and then fail at run time on exactly - that reference (#465). A warning that only reaches the log stream is not - a warning: `orchestrator validate` prints to stdout, and a script - capturing stdout saw nothing at all. - - Informational findings are excluded: "tool is available" and "execution - order computed" are not things anyone needs told. - """ - report = getattr(compiler, "validation_report", None) - if report is None: - return [] - return [issue for issue in report.issues if issue.is_error or issue.is_warning] - - -def _issue_payload(issue): - """One finding as stable, machine-readable fields. - - Field names are part of the interface: anything reading this should not - have to parse the human message to learn which step or reference is at - fault. - """ - metadata = issue.metadata or {} - return { - "code": issue.code, - "severity": issue.severity.value, - "category": issue.category, - "step": metadata.get("step", issue.component), - "parameter_path": metadata.get("parameter_path", issue.path), - "referenced_step": metadata.get("referenced_step"), - "referenced_field": metadata.get("referenced_field"), - "message": issue.message, - "suggestions": list(issue.suggestions or []), - } - - @cli.command("validate") @click.argument("pipeline_file", type=click.Path(exists=True, dir_okay=False)) @click.option("--json", "as_json", is_flag=True, @@ -414,72 +374,61 @@ def _issue_payload(issue): @verbose_option def validate_pipeline(pipeline_file: str, verbose: bool, as_json: bool): """Compile a pipeline without running it, and report its task graph.""" - import asyncio import json as _json if verbose: setup_logging(verbose=True) - from .compiler.yaml_compiler import YAMLCompiler - - compiler = YAMLCompiler() - - async def _compile(): - with open(pipeline_file) as fh: - return await compiler.compile(fh.read(), {}) + from .validation.pipeline_report import validate_pipeline_file try: - pipeline = asyncio.run(_compile()) + result = validate_pipeline_file(pipeline_file) except KeyboardInterrupt: click.echo("Interrupted.", err=True) sys.exit(EXIT_INTERRUPTED) - except Exception as exc: + + if not result.valid: if as_json: click.echo(_json.dumps({ "pipeline_file": pipeline_file, "valid": False, - "error": f"{type(exc).__name__}: {exc}", - "findings": [_issue_payload(i) for i in _reportable_issues(compiler)], + "error": result.error, + "findings": [f.as_dict() for f in result.findings], }, indent=2)) else: - click.echo(f"{type(exc).__name__}: {exc}", err=True) + click.echo(result.error, err=True) sys.exit(EXIT_VALIDATION_ERROR) - tasks = getattr(pipeline, "tasks", {}) or {} - findings = _reportable_issues(compiler) + tasks = result.tasks + findings = result.findings if as_json: click.echo(_json.dumps({ "pipeline_file": pipeline_file, "valid": True, - "pipeline": getattr(pipeline, "id", None), - "tasks": { - task_id: list(getattr(task, "dependencies", []) or []) - for task_id, task in tasks.items() - }, - "findings": [_issue_payload(issue) for issue in findings], + "pipeline": result.pipeline_id, + "tasks": tasks, + "findings": [f.as_dict() for f in findings], }, indent=2)) sys.exit(EXIT_OK) - warnings = [issue for issue in findings if issue.is_warning] + warnings = result.warnings summary = f"✓ {pipeline_file} is valid" if warnings: summary += f" with {len(warnings)} warning{'s' if len(warnings) > 1 else ''}" click.echo(summary) - click.echo(f" pipeline: {getattr(pipeline, 'id', '')}") + click.echo(f" pipeline: {result.pipeline_id or ''}") click.echo(f" tasks: {len(tasks)}") - for task_id in tasks: - task = tasks[task_id] - deps = getattr(task, "dependencies", []) or [] + for task_id, deps in tasks.items(): suffix = f" <- {', '.join(deps)}" if deps else "" click.echo(f" - {task_id}{suffix}") for issue in findings: click.echo("") - where = issue.path or issue.component - click.echo(f"{issue.severity.value}[{issue.code}] {where}:") + where = issue.parameter_path or issue.step + click.echo(f"{issue.severity}[{issue.code}] {where}:") click.echo(f" {issue.message}") - for suggestion in issue.suggestions or []: + for suggestion in issue.suggestions: click.echo(f" suggestion: {suggestion}") sys.exit(EXIT_OK) diff --git a/src/orchestrator/compiler/yaml_compiler.py b/src/orchestrator/compiler/yaml_compiler.py index 1cca989..d2e8dcb 100644 --- a/src/orchestrator/compiler/yaml_compiler.py +++ b/src/orchestrator/compiler/yaml_compiler.py @@ -455,7 +455,8 @@ async def _validate_templates( component=error.context_path or "unknown", message=error.message, path=error.context_path, - suggestions=error.suggestions + suggestions=error.suggestions, + code=error.error_type, ) self.validation_report.add_issue(issue) @@ -466,7 +467,8 @@ async def _validate_templates( component=warning.context_path or "unknown", message=warning.message, path=warning.context_path, - suggestions=warning.suggestions + suggestions=warning.suggestions, + code=warning.error_type, ) self.validation_report.add_issue(issue) diff --git a/src/orchestrator/core/step_fields.py b/src/orchestrator/core/step_fields.py index 61df6c9..eed1937 100644 --- a/src/orchestrator/core/step_fields.py +++ b/src/orchestrator/core/step_fields.py @@ -64,18 +64,63 @@ "steps", ) -#: Named so a test can assert they are never scanned, and so the reason is -#: written down rather than implied by absence. -INERT_STEP_FIELDS: FrozenSet[str] = frozenset({ - "id", # the step's own name +#: Prose. Nothing renders it and nothing acts on it, so a template here is the +#: author's mistake to know about but not a defect in the pipeline: the braces +#: appear in a log line and everything still runs. +INERT_PROSE_STEP_FIELDS: FrozenSet[str] = frozenset({ "name", # human-readable label, copied verbatim "description", # prose - "metadata", # arbitrary author data - "tool", # a registry key, not a template - "dependencies", # already read, with the `declared` origin +}) + +#: Also never rendered -- and that makes a template here an error, not a note. +#: These fields *name* things: a step, a tool in the registry, a dependency to +#: schedule against. A literal `{{ x }}` names nothing, so the pipeline is +#: already broken; warning about the wording and calling it valid says the +#: opposite. +NON_RENDERED_STRUCTURAL_STEP_FIELDS: FrozenSet[str] = frozenset({ + "id", # the step's own name + "tool", # a registry key + "dependencies", # read with the `declared` origin "depends_on", }) +#: `metadata` is arbitrary author data *except* for the keys the runtime +#: reads. Those are not prose: an unrendered `{{ ... }}` is handed to control +#: code as a literal string, so `goto: "{{ nosuch }}"` sends execution to a +#: step named `{{ nosuch }}` and `timeout: "{{ t }}"` is a timeout of that +#: text. Each key here has a runtime read behind it: +#: +#: goto orchestrator.py:1074, control_flow/dynamic_flow.py:53 +#: priority orchestrator.py:1345 +#: requires_model orchestrator.py:2599 +#: dynamic_dependencies orchestrator.py:1166 +#: on_failure orchestrator.py:1111 +#: timeout orchestrator.py:1888 +#: max_iterations orchestrator.py:1001 +#: condition runtime/orchestrator_integration.py:123 +#: output_schema adapters/enhanced_langgraph_adapter.py:222 +#: produces auto_resolution/requirements_analyzer.py:219 +#: required_capabilities core/control_system.py:130 +#: validation compiler/schema_resolver.py:149 +#: +#: Keys the compiler *writes* to metadata (`step_type`, `retry_count`, the +#: loop bookkeeping) are absent: an author does not supply them, so a template +#: in one is not a case that arises. +OPERATIONAL_METADATA_KEYS: FrozenSet[str] = frozenset({ + "goto", "priority", "requires_model", "dynamic_dependencies", + "on_failure", "timeout", "max_iterations", "condition", + "output_schema", "produces", "required_capabilities", "validation", +}) + +#: Everything a step carries that the runtime does not render, whatever the +#: consequence of writing a template in it. Dependency inference only needs +#: "not scanned"; the distinctions above are for diagnostics. +INERT_STEP_FIELDS: FrozenSet[str] = ( + INERT_PROSE_STEP_FIELDS + | NON_RENDERED_STRUCTURAL_STEP_FIELDS + | frozenset({"metadata"}) +) + #: The same distinction one level up. A pipeline's own `name` and #: `description` are prose about the pipeline; nothing renders them either, so #: `name: "{{ nosuch }}"` at the top of a document was failing validation for a diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index fd3fbfd..9efdca5 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/src/orchestrator/validation/data_flow_validator.py @@ -615,6 +615,14 @@ def _suggest_similar_names(self, target: str, available: List[str]) -> List[str] """Suggest similar names for typos.""" suggestions = [] target_lower = target.lower() + + # Sorted, because callers pass names collected in a set and this + # function both emits in iteration order and truncates to three. Two + # identical `orchestrator validate` runs offered the same three + # suggestions in different orders, and on a longer candidate list + # would have offered *different* three. The tiers below still rank the + # result; this only makes ties break the same way every time. + available = sorted(available) # Exact match (case insensitive) for name in available: diff --git a/src/orchestrator/validation/pipeline_report.py b/src/orchestrator/validation/pipeline_report.py new file mode 100644 index 0000000..b35562d --- /dev/null +++ b/src/orchestrator/validation/pipeline_report.py @@ -0,0 +1,169 @@ +"""Validating a pipeline, with its findings, from Python. + +`orchestrator validate` has reported structured findings since #467. The +Python API had no equivalent: `PipelineAPI.validate_yaml` returns a bare +`bool`, so a caller embedding the orchestrator could learn *that* a pipeline +was rejected and nothing about why -- and could not see warnings at all, which +is where "this reference could not be checked" lives. + +The findings were not missing, only private: `cli._reportable_issues` and +`cli._issue_payload` built them for the JSON output and nothing else could +reach them. A second implementation for the API would be a second thing to +drift; this module is the one implementation, and the CLI formats what it +returns. + +The payload field names are interface. Anything consuming a finding should not +have to parse the human message to learn which step or reference is at fault. +""" + +from __future__ import annotations + +import asyncio +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Dict, List, Optional, Tuple, Union + + +@dataclass(frozen=True) +class Finding: + """One validation finding, in stable machine-readable fields.""" + + code: str + severity: str + category: Optional[str] = None + step: Optional[str] = None + parameter_path: Optional[str] = None + referenced_step: Optional[str] = None + referenced_field: Optional[str] = None + message: str = "" + suggestions: Tuple[str, ...] = () + + @property + def is_error(self) -> bool: + return self.severity == "error" + + @property + def is_warning(self) -> bool: + return self.severity == "warning" + + def as_dict(self) -> Dict[str, Any]: + return { + "code": self.code, + "severity": self.severity, + "category": self.category, + "step": self.step, + "parameter_path": self.parameter_path, + "referenced_step": self.referenced_step, + "referenced_field": self.referenced_field, + "message": self.message, + "suggestions": list(self.suggestions), + } + + +@dataclass(frozen=True) +class PipelineValidation: + """The result of validating one pipeline document.""" + + valid: bool + findings: Tuple[Finding, ...] = () + pipeline_id: Optional[str] = None + #: Step id -> its dependencies, so a caller can check the graph the + #: compiler actually built rather than re-deriving it. + tasks: Dict[str, List[str]] = field(default_factory=dict) + #: The exception that stopped compilation, if one did. + error: Optional[str] = None + + @property + def errors(self) -> Tuple[Finding, ...]: + return tuple(f for f in self.findings if f.is_error) + + @property + def warnings(self) -> Tuple[Finding, ...]: + return tuple(f for f in self.findings if f.is_warning) + + def as_dict(self) -> Dict[str, Any]: + return { + "valid": self.valid, + "pipeline": self.pipeline_id, + "tasks": self.tasks, + "findings": [f.as_dict() for f in self.findings], + **({"error": self.error} if self.error is not None else {}), + } + + +def _findings_from(compiler) -> Tuple[Finding, ...]: + """Findings a successful validation should still tell the caller about. + + The compiler has always collected these; nothing displayed them. A + pipeline could report valid while carrying a warning saying a reference + could not be checked -- and then fail at run time on exactly that + reference (#465). + + Informational findings are excluded: "tool is available" and "execution + order computed" are not things anyone needs told. + """ + report = getattr(compiler, "validation_report", None) + if report is None: + return () + return tuple( + _finding(issue) + for issue in report.issues + if issue.is_error or issue.is_warning + ) + + +def _finding(issue) -> Finding: + metadata = issue.metadata or {} + severity = issue.severity + return Finding( + code=issue.code, + severity=getattr(severity, "value", severity), + category=issue.category, + step=metadata.get("step", issue.component), + parameter_path=metadata.get("parameter_path", issue.path), + referenced_step=metadata.get("referenced_step"), + referenced_field=metadata.get("referenced_field"), + message=issue.message, + suggestions=tuple(issue.suggestions or ()), + ) + + +async def validate_pipeline_text(yaml_text: str) -> PipelineValidation: + """Compile `yaml_text` without running it and report what was found. + + Compilation failure is a result, not an exception: a caller validating + user input wants the findings alongside the failure, and raising would + discard them. + """ + from ..compiler.yaml_compiler import YAMLCompiler + + compiler = YAMLCompiler() + try: + pipeline = await compiler.compile(yaml_text, {}) + except Exception as exc: + return PipelineValidation( + valid=False, + findings=_findings_from(compiler), + error=f"{type(exc).__name__}: {exc}", + ) + + tasks = getattr(pipeline, "tasks", {}) or {} + return PipelineValidation( + valid=True, + findings=_findings_from(compiler), + pipeline_id=getattr(pipeline, "id", None), + tasks={ + task_id: list(getattr(task, "dependencies", []) or []) + for task_id, task in tasks.items() + }, + ) + + +def validate_pipeline_file(path: Union[str, Path]) -> PipelineValidation: + """`validate_pipeline_text` for a file on disk, without an event loop. + + Provided because the common case -- checking a pipeline someone wrote -- + is synchronous, and requiring callers to build an event loop to learn + whether a file is valid is why the boolean survived so long. + """ + return asyncio.run(validate_pipeline_text(Path(path).read_text())) diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index e7f29ce..a01b24d 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/src/orchestrator/validation/template_validator.py @@ -19,7 +19,12 @@ from ..core.runtime_context import BARE_RUNTIME_NAMES, RUNTIME_NAMESPACE from ..core.loop_contracts import ALL_BINDINGS, LoopContract, contracts_for -from ..core.step_fields import INERT_PIPELINE_FIELDS, INERT_STEP_FIELDS +from ..core.step_fields import ( + INERT_PIPELINE_FIELDS, + INERT_PROSE_STEP_FIELDS, + NON_RENDERED_STRUCTURAL_STEP_FIELDS, + OPERATIONAL_METADATA_KEYS, +) from ..core.template_globals import ( ALL_LOOP_VARIABLES, DOLLAR_LOOP_VARIABLES, @@ -51,6 +56,49 @@ "the braces appear literally in the output" ) +#: How a field being unrendered should be reported. "Unrendered" alone does +#: not settle it: the first version of this treated every such field as prose +#: and warned, which said a `goto` sending execution to a step literally named +#: `{{ nosuch }}` was a wording problem. +_PROSE = "prose" +_STRUCTURAL = "structural" +_OPERATIONAL = "operational" + +_INERT_ERROR_TYPE = { + _STRUCTURAL: "template_in_structural_field", + _OPERATIONAL: "template_in_operational_metadata", +} + +_INERT_ERROR_MESSAGE = { + _STRUCTURAL: ( + "'{field}' names a step, tool or dependency and is never rendered, so " + "this template cannot resolve to the name it is standing in for" + ), + _OPERATIONAL: ( + "metadata '{field}' is read by the runtime and is never rendered, so " + "the literal template text is what the runtime would act on" + ), +} + +_INERT_ERROR_SUGGESTION = { + _STRUCTURAL: "Write the literal name here", + _OPERATIONAL: "Write a literal value, or compute it in a rendered field", +} + + +def _classify(path: str, key: str, is_step: bool): + """How a field's unrenderedness should be reported, or None if it renders.""" + if is_step: + if key in INERT_PROSE_STEP_FIELDS: + return (key, _PROSE) + if key in NON_RENDERED_STRUCTURAL_STEP_FIELDS: + return (key, _STRUCTURAL) + if key == "metadata": + return (key, _PROSE) + if not path and key in INERT_PIPELINE_FIELDS: + return (key, _PROSE) + return None + def _binding_set(value: Union[bool, FrozenSet[str], None]) -> FrozenSet[str]: """Normalise the loop-scope argument to a set of names. @@ -411,9 +459,16 @@ def _validate_variables( if loop_var in template ] - # Combine both sets of variables - all_var_names = set(var_names) | set(loop_var_matches) - + # Combine both sets of variables. + # + # Sorted, because findings are emitted in this order and a set of + # strings iterates by hash. Two identical `orchestrator validate` + # runs on the same file produced the same 44 findings in different + # orders, so any caller diffing runs, pinning output, or reporting + # "the first problem" saw noise. `PYTHONHASHSEED` differs per + # process, which is why the in-process check missed it. + all_var_names = sorted(set(var_names) | set(loop_var_matches)) + for var_name in all_var_names: used_variables.add(var_name) @@ -617,7 +672,7 @@ def _validate_object_templates( undefined_variables: Set, loop_bindings: FrozenSet[str] = frozenset(), loop_scope: Optional[Tuple[LoopContract, str, FrozenSet[str]]] = None, - inert_field: Optional[str] = None, + inert_field: Optional[Tuple[str, str]] = None, ): """Recursively validate templates in an object. @@ -627,26 +682,36 @@ def _validate_object_templates( not of the step: a `create_parallel_queue`'s `on` resolves before any item exists while the action list beside it runs per item. - `inert_field` names the step field the runtime copies verbatim, if - this walk is inside one. Nothing substitutes into it, so a reference - there cannot be undefined and cannot be resolved later. The *field* is - carried rather than a flag so a nested value reports `metadata` -- the - field that is inert -- instead of whichever key it sits under. + `inert_field` is `(field, kind)` for the unrendered step field this + walk is inside, if any -- see `_classify`. It names the field rather + than a flag so a nested value reports `metadata` instead of whichever + key it sits under, and carries the kind because being unrendered is a + warning in prose and an error in a field the runtime acts on. """ if isinstance(obj, str): # Check if this contains templates if '{{' in obj or '{%' in obj: if inert_field: - warnings.append(TemplateValidationError( + field, kind = inert_field + if kind == _PROSE: + warnings.append(TemplateValidationError( + template=obj, + error_type="inert_field_template", + message=_INERT_TEMPLATE_MESSAGE.format(field=field), + context_path=path, + severity="warning", + suggestions=[ + "Move the reference to a field that is rendered " + "(parameters, action, location), or remove the braces" + ], + )) + return + errors.append(TemplateValidationError( template=obj, - error_type="inert_field_template", - message=_INERT_TEMPLATE_MESSAGE.format(field=inert_field), + error_type=_INERT_ERROR_TYPE[kind], + message=_INERT_ERROR_MESSAGE[kind].format(field=field), context_path=path, - severity="warning", - suggestions=[ - "Move the reference to a field that is rendered " - "(parameters, action, location), or remove the braces" - ], + suggestions=[_INERT_ERROR_SUGGESTION[kind]], )) return result = self.validate_template( @@ -663,7 +728,13 @@ def _validate_object_templates( # `action_loop` look like a second, separate loop, which replaced # the queue's scope with the action loop's and let `{{ item }}` # through in the `on` expression that generates the queue. - is_step = bool(_STEP_PATH.search(path)) + # Nothing inside an inert field is a step, whatever it looks like. + # `metadata` holds arbitrary author data, so a `metadata.steps` + # list carrying `for_each` and `while` keys was being read as + # pipeline structure and reported as an ambiguous loop -- inside a + # subtree this module has just declared the runtime copies + # verbatim. + is_step = inert_field is None and bool(_STEP_PATH.search(path)) declared = contracts_for(obj) if is_step else () if len(declared) > 1: # Which construct wins is decided by declaration order in @@ -692,11 +763,17 @@ def _validate_object_templates( for key, value in obj.items(): new_path = f"{path}.{key}" if path else key - inert_here = ( - (is_step and key in INERT_STEP_FIELDS) - or (not path and key in INERT_PIPELINE_FIELDS) - ) - child_inert = inert_field or (key if inert_here else None) + child_inert = inert_field or _classify(path, key, is_step) + if ( + inert_field is not None + and path.endswith(".metadata") + and key in OPERATIONAL_METADATA_KEYS + ): + # A reserved key *inside* metadata. The object itself is + # arbitrary author data; these particular keys are read by + # control code, so an unrendered template in one is handed + # to it as a literal string. + child_inert = (key, _OPERATIONAL) child_bindings, child_scope = loop_bindings, loop_scope if loop_scope is not None: contract, prefix, enclosing = loop_scope diff --git a/src/orchestrator/validation/validation_report.py b/src/orchestrator/validation/validation_report.py index a293a6b..da88f7e 100644 --- a/src/orchestrator/validation/validation_report.py +++ b/src/orchestrator/validation/validation_report.py @@ -643,8 +643,17 @@ def clear(self): # Convenience functions for creating validation issues def create_template_issue(severity: ValidationSeverity, component: str, message: str, - path: Optional[str] = None, suggestions: Optional[List[str]] = None) -> ValidationIssue: - """Create a template validation issue.""" + path: Optional[str] = None, suggestions: Optional[List[str]] = None, + code: str = "template_error") -> ValidationIssue: + """Create a template validation issue. + + `code` defaults to the generic `template_error` for callers that have no + more specific one, but the template validator distinguishes its findings + -- `loop_variable_wrong_construct` is not `inert_field_template` is not + `template_in_operational_metadata` -- and flattening them here meant a + consumer had to parse the human message to tell them apart, which is + exactly what the structured payload exists to avoid. + """ return ValidationIssue( severity=severity, category="template", @@ -652,7 +661,7 @@ def create_template_issue(severity: ValidationSeverity, component: str, message: message=message, path=path, suggestions=suggestions or [], - code="template_error" + code=code ) diff --git a/tests/test_inert_field_diagnostics.py b/tests/test_inert_field_diagnostics.py index 11e6963..ce34634 100644 --- a/tests/test_inert_field_diagnostics.py +++ b/tests/test_inert_field_diagnostics.py @@ -25,7 +25,9 @@ import pytest from orchestrator.core.step_fields import ( - INERT_STEP_FIELDS, + INERT_PROSE_STEP_FIELDS, + NON_RENDERED_STRUCTURAL_STEP_FIELDS, + OPERATIONAL_METADATA_KEYS, RENDERABLE_STEP_FIELDS, ) from orchestrator.validation.template_validator import TemplateValidator @@ -33,10 +35,10 @@ pytestmark = [pytest.mark.contract] -#: `id` names the step and `dependencies`/`depends_on` hold step ids. They are -#: inert for the same reason, but a template in one is a structural error -#: rather than prose, so they are not probed as free text here. -PROSE_FIELDS = sorted(INERT_STEP_FIELDS - {"id", "dependencies", "depends_on", "tool"}) +#: Fields that are prose: unrendered, and harmless. `metadata` joins them +#: because arbitrary author data is prose too -- its *reserved* keys are not, +#: and those are covered separately below. +PROSE_FIELDS = sorted(INERT_PROSE_STEP_FIELDS | {"metadata"}) def _validate(step, context=None): @@ -150,3 +152,85 @@ def test_that_same_pipeline_validates(tmp_path): {}, ) assert result.is_valid, [(e.error_type, e.context_path) for e in result.errors] + + +# --------------------------------------------------------------------------- +# Unrendered does not mean harmless +# --------------------------------------------------------------------------- +# +# The first version of this module treated every unrendered field as prose and +# warned. That said a `goto` sending execution to a step literally named +# `{{ nosuch }}` was a wording problem, and reported a pipeline carrying it as +# valid. + + +@pytest.mark.parametrize("field", sorted(NON_RENDERED_STRUCTURAL_STEP_FIELDS)) +def test_a_template_in_a_structural_field_is_an_error(field): + """These fields *name* things -- a step, a tool, a dependency. A literal + `{{ x }}` names nothing, so the pipeline is already broken.""" + value = ["{{ nosuch }}"] if field in ("dependencies", "depends_on") else "{{ nosuch }}" + result = _validate({"id": "a", field: value}) + assert not result.is_valid, f"a template in '{field}' cannot resolve to a name" + assert "template_in_structural_field" in [e.error_type for e in result.errors] + + +@pytest.mark.parametrize("key", sorted(OPERATIONAL_METADATA_KEYS)) +def test_a_template_in_runtime_read_metadata_is_an_error(key): + """Each of these keys has a runtime read behind it, so the literal + template text is what control code would act on.""" + result = _validate({"id": "a", "metadata": {key: "{{ nosuch }}"}}) + assert not result.is_valid, f"metadata '{key}' is read by the runtime" + assert "template_in_operational_metadata" in [e.error_type for e in result.errors] + + +def test_arbitrary_metadata_is_still_prose(): + """The split must not swallow the case it started from: a note an author + wrote for themselves is not a defect.""" + result = _validate({"id": "a", "metadata": {"note": "{{ nosuch }}"}}) + assert result.is_valid, [(e.error_type, e.message) for e in result.errors] + assert "inert_field_template" in [w.error_type for w in result.warnings] + + +def test_an_operational_key_is_only_operational_inside_metadata(): + """`priority` nested deeper is somebody's data structure, not the key the + runtime reads.""" + result = _validate({"id": "a", "metadata": {"notes": {"priority": "{{ nosuch }}"}}}) + assert result.is_valid, [(e.error_type, e.message) for e in result.errors] + + +def test_metadata_is_not_traversed_as_pipeline_structure(): + """`metadata` holds arbitrary author data, so a key named `steps` inside it + is data. It was being walked as pipeline structure, and a dict carrying + `for_each` and `while` reported an ambiguous loop -- from inside a subtree + this module had just declared the runtime copies verbatim.""" + result = _validate({"id": "a", "metadata": {"steps": [{"for_each": "x", "while": "y"}]}}) + assert "ambiguous_loop_construct" not in [e.error_type for e in result.errors] + assert result.is_valid, [(e.error_type, e.context_path) for e in result.errors] + + +def test_the_three_classes_are_disjoint(): + """A field in two of them would be reported by whichever check ran first.""" + assert not (INERT_PROSE_STEP_FIELDS & NON_RENDERED_STRUCTURAL_STEP_FIELDS) + assert not (set(RENDERABLE_STEP_FIELDS) & INERT_PROSE_STEP_FIELDS) + assert not (set(RENDERABLE_STEP_FIELDS) & NON_RENDERED_STRUCTURAL_STEP_FIELDS) + + +@pytest.mark.e2e +def test_the_specific_diagnostic_code_survives_to_the_api(tmp_path): + """`create_template_issue` hardcoded `template_error`, so every template + finding arrived as the same code and a consumer had to read the prose to + tell an inert-field note from a loop-scope error.""" + from orchestrator.validation.pipeline_report import validate_pipeline_file + + document = tmp_path / "p.yaml" + document.write_text( + "id: codes\nname: codes\nsteps:\n" + ' - id: a\n name: "{{ nosuch }}"\n tool: filesystem\n' + " action: write\n parameters:\n" + f' path: "{tmp_path}/x.txt"\n content: hi\n' + ) + result = validate_pipeline_file(document) + assert result.valid, result.error + assert "inert_field_template" in [f.code for f in result.findings], [ + f.code for f in result.findings + ] diff --git a/tests/test_validation_api.py b/tests/test_validation_api.py new file mode 100644 index 0000000..67b405a --- /dev/null +++ b/tests/test_validation_api.py @@ -0,0 +1,182 @@ +"""Validation findings reach a Python caller, not only the terminal. + +`orchestrator validate --json` has emitted structured findings since #467. The +Python API had `PipelineAPI.validate_yaml`, which returns a bare `bool`: a +caller embedding the orchestrator could learn *that* a document was rejected +and nothing about why, and could not see warnings at all -- which is where +"this reference could not be checked" lives, the warning that precedes the +run-time failure in #465. + +The findings existed; they were private to `cli.py`. So the risk in exposing +them was building a *second* implementation that drifts from the one the CLI +prints, which is the class of bug #466 removed for dependencies. There is one +implementation, and `test_the_cli_and_the_api_report_the_same_findings` is +what holds it to that. +""" + +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from orchestrator.validation.pipeline_report import ( + Finding, + validate_pipeline_file, + validate_pipeline_text, +) + +pytestmark = [pytest.mark.contract] + +REPO = Path(__file__).resolve().parent.parent +SUPPORTED = REPO / "examples" / "supported" / "01_hello_filesystem.yaml" + +#: A document that validates *and* carries findings. Every assertion about +#: findings needs one: the hermetic example above produces none, so a suite +#: written only against it passes just as happily when warnings are dropped +#: on the floor -- which is what the first version of this file did. +WITH_FINDINGS = REPO / "examples" / "auto_tags_demo.yaml" + +#: The payload's field names are interface: a consumer must not have to parse +#: the human message to learn which step or reference is at fault. Asserted as +#: an exact set so removing or renaming one is a deliberate act. +FINDING_FIELDS = { + "code", "severity", "category", "step", "parameter_path", + "referenced_step", "referenced_field", "message", "suggestions", +} + + +def test_a_valid_pipeline_reports_its_graph(): + result = validate_pipeline_file(SUPPORTED) + assert result.valid, result.error + assert result.pipeline_id == "hello_filesystem" + assert result.tasks["read_back"] == ["write_greeting"], result.tasks + assert result.error is None + + +def test_a_broken_pipeline_is_a_result_not_an_exception(): + """A caller validating user input wants the findings alongside the + failure. Raising would discard them at the moment they are most useful.""" + import asyncio + + result = asyncio.run(validate_pipeline_text("id: broken\nsteps: [[[[")) + assert not result.valid + assert result.error, "the failure must say what happened" + + +def test_warnings_reach_a_python_caller(): + """The reported gap. `validate_yaml` returns a bool, so a warning saying + a reference could not be checked -- the one that precedes the run-time + failure in #465 -- was invisible to anything but the terminal.""" + result = validate_pipeline_file(WITH_FINDINGS) + assert result.valid, result.error + assert result.warnings, "this document warns; the API reported nothing" + assert any( + "will be resolved at runtime" in w.message for w in result.warnings + ), [w.message for w in result.warnings] + + +def test_a_warning_does_not_make_a_document_invalid(): + """Warnings inform; they do not block. Conflating them would reject + documents that run.""" + assert validate_pipeline_file(WITH_FINDINGS).valid + + +def test_findings_carry_stable_named_fields(): + result = validate_pipeline_file(WITH_FINDINGS) + assert result.findings, "nothing to check the field names of" + for finding in result.findings: + assert set(finding.as_dict()) == FINDING_FIELDS, finding.as_dict() + + +def test_errors_and_warnings_are_separable(): + """A caller that wants to block on errors and log warnings should not have + to string-match a severity out of a message.""" + findings = ( + Finding(code="a", severity="error", message="x"), + Finding(code="b", severity="warning", message="y"), + ) + from orchestrator.validation.pipeline_report import PipelineValidation + + result = PipelineValidation(valid=False, findings=findings) + assert [f.code for f in result.errors] == ["a"] + assert [f.code for f in result.warnings] == ["b"] + assert result.errors[0].is_error and result.warnings[0].is_warning + + +def test_the_same_document_yields_the_same_findings_in_the_same_order(): + """Order is part of the contract: a caller diffing two runs, or a test + pinning output, cannot use a set that reshuffles.""" + first = validate_pipeline_file(WITH_FINDINGS) + second = validate_pipeline_file(WITH_FINDINGS) + assert first.findings, "an empty list is trivially stable" + assert [f.as_dict() for f in first.findings] == [ + f.as_dict() for f in second.findings + ] + + +@pytest.mark.e2e +def test_findings_are_stable_across_processes(): + """The check above cannot catch this on its own. + + Findings were emitted while iterating sets, and `PYTHONHASHSEED` is fixed + for the life of a process -- so two calls in one interpreter agreed while + two `orchestrator validate` runs produced the same 44 findings in + different orders, and offered the same three "did you mean" suggestions + permuted. On a longer candidate list, truncating to three would have + offered *different* suggestions run to run. + """ + digests = {_cli_findings_digest() for _ in range(3)} + assert len(digests) == 1, "validate is not reproducible run to run" + + +def _cli_findings_digest() -> str: + import hashlib + + proc = subprocess.run( + [sys.executable, "-m", "orchestrator.cli", "validate", "--json", str(WITH_FINDINGS)], + cwd=str(REPO), env=_cli_env(), capture_output=True, text=True, timeout=300, + ) + assert proc.returncode == 0, proc.stdout[-800:] + proc.stderr[-800:] + findings = json.loads(proc.stdout)["findings"] + assert findings, "nothing to be stable about" + return hashlib.sha256(json.dumps(findings).encode()).hexdigest() + + +def _cli_env() -> dict: + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") + env["ORCHESTRATOR_AUTO_INSTALL"] = "0" + return env + + +@pytest.mark.e2e +def test_the_cli_and_the_api_report_the_same_findings(): + """The guard against a second implementation. + + The CLI's findings were private helpers in `cli.py`; exposing them to + Python by reimplementing would have produced two things to drift apart. + """ + proc = subprocess.run( + [sys.executable, "-m", "orchestrator.cli", "validate", "--json", str(WITH_FINDINGS)], + cwd=str(REPO), env=_cli_env(), capture_output=True, text=True, timeout=300, + ) + assert proc.returncode == 0, proc.stdout[-800:] + proc.stderr[-800:] + from_cli = json.loads(proc.stdout) + + from_api = validate_pipeline_file(WITH_FINDINGS) + assert from_api.findings, "comparing two empty lists proves nothing" + assert from_cli["valid"] == from_api.valid + assert from_cli["pipeline"] == from_api.pipeline_id + assert from_cli["tasks"] == from_api.tasks + assert from_cli["findings"] == [f.as_dict() for f in from_api.findings] + + +def test_the_package_exports_them(): + """A caller should not have to know which module they live in.""" + import orchestrator + + assert orchestrator.validate_pipeline_file is validate_pipeline_file + assert orchestrator.Finding is Finding