From 59268b9cd090265cabd82bc711dba6a7eefaea14 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 3 Aug 2026 23:43:20 -0400 Subject: [PATCH 1/2] Show the warnings the compiler already had MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pipeline could print ✓ p.yaml is valid while carrying a warning that a reference could not be checked, and then fail at run time on exactly that reference (#465). The compiler had collected the finding all along; nothing displayed it. It went to the log stream, which validate does not write to and a script capturing stdout never sees. A successful validate now prints its warnings, with their codes, and says how many in the summary line. Informational findings stay out: 'tool is available' and 'execution order computed' are not things anyone needs told, and reporting that would turn the channel into noise. --json emits the whole result as one document, findings included, with field names that are an interface rather than English to be parsed: code, severity, category, step, parameter_path, referenced_step, referenced_field, message, suggestions. Data-flow findings now carry the referenced step and field structurally instead of only in prose. Along the way: cli.py defined 'def list()' at module scope, rebinding the builtin for the whole file. A click Command is callable, so 'list(...)' did not build a list -- it invoked the command, printed the configured providers and exited before its own output. --json emitted nothing and looked like a registration problem. The function is now list_keys with the CLI name passed to the decorator, and a test refuses any module-level name in cli.py that shadows a builtin. Co-Authored-By: Claude Opus 5 (1M context) --- src/orchestrator/cli.py | 100 +++++++++- src/orchestrator/compiler/yaml_compiler.py | 34 +++- tests/test_validation_output.py | 208 +++++++++++++++++++++ 3 files changed, 325 insertions(+), 17 deletions(-) create mode 100644 tests/test_validation_output.py diff --git a/src/orchestrator/cli.py b/src/orchestrator/cli.py index 4c82d5b..cc4ad2c 100644 --- a/src/orchestrator/cli.py +++ b/src/orchestrator/cli.py @@ -99,9 +99,15 @@ def setup(): subprocess.run([sys.executable, str(setup_script)]) -@keys.command() -def list(): - """Show configured providers (not the keys).""" +@keys.command("list") +def list_keys(): + """Show configured providers (not the keys). + + Named `list_keys`, not `list`: at module scope `def list()` rebinds the + builtin for the whole file, and a click Command is callable -- so a later + `list(...)` silently *invokes this command*, printed the provider list and + exited before its own output. The CLI name is unchanged. + """ try: providers = get_configured_providers() if providers: @@ -361,20 +367,64 @@ 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, + help="Emit the result and every finding as JSON on stdout.") @verbose_option -def validate_pipeline(pipeline_file: str, verbose: bool): +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(): - compiler = YAMLCompiler() with open(pipeline_file) as fh: return await compiler.compile(fh.read(), {}) @@ -384,11 +434,38 @@ async def _compile(): click.echo("Interrupted.", err=True) sys.exit(EXIT_INTERRUPTED) except Exception as exc: - click.echo(f"{type(exc).__name__}: {exc}", err=True) + 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)], + }, indent=2)) + else: + click.echo(f"{type(exc).__name__}: {exc}", err=True) sys.exit(EXIT_VALIDATION_ERROR) tasks = getattr(pipeline, "tasks", {}) or {} - click.echo(f"✓ {pipeline_file} is valid") + findings = _reportable_issues(compiler) + + 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], + }, indent=2)) + sys.exit(EXIT_OK) + + warnings = [issue for issue in findings if issue.is_warning] + 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" tasks: {len(tasks)}") for task_id in tasks: @@ -396,6 +473,15 @@ async def _compile(): deps = getattr(task, "dependencies", []) or [] 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}:") + click.echo(f" {issue.message}") + for suggestion in issue.suggestions or []: + 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 c65cced..1cca989 100644 --- a/src/orchestrator/compiler/yaml_compiler.py +++ b/src/orchestrator/compiler/yaml_compiler.py @@ -36,6 +36,28 @@ logger = logging.getLogger(__name__) + +def _data_flow_metadata(finding) -> Dict[str, Any]: + """Stable, machine-readable fields for one data-flow finding. + + The step, the parameter, the step referred to and the field referred to + are all things a consumer should be able to read directly. Leaving them + only in the prose message forces anything programmatic to parse English, + and the message is not an interface. + """ + reference = finding.variable_reference or "" + _, _, field = reference.partition(".") + return { + "error_type": finding.error_type, + "variable_reference": finding.variable_reference, + "source_task": finding.source_task, + "step": finding.task_id, + "parameter_path": finding.parameter_name, + "referenced_step": finding.source_task, + "referenced_field": field or None, + } + + class AutoTagNotFoundError(YAMLCompilerError): """Raised when AUTO tag resolution fails.""" @@ -740,11 +762,7 @@ async def _validate_data_flow(self, pipeline_def: Dict[str, Any]) -> None: code=f"data_flow_{error.error_type}", path=error.parameter_name, suggestions=error.suggestions, - metadata={ - "error_type": error.error_type, - "variable_reference": error.variable_reference, - "source_task": error.source_task - } + metadata=_data_flow_metadata(error) ) self.validation_report.add_issue(issue) @@ -758,11 +776,7 @@ async def _validate_data_flow(self, pipeline_def: Dict[str, Any]) -> None: code=f"data_flow_{warning.error_type}", path=warning.parameter_name, suggestions=warning.suggestions, - metadata={ - "error_type": warning.error_type, - "variable_reference": warning.variable_reference, - "source_task": warning.source_task - } + metadata=_data_flow_metadata(warning) ) self.validation_report.add_issue(issue) diff --git a/tests/test_validation_output.py b/tests/test_validation_output.py new file mode 100644 index 0000000..62d377f --- /dev/null +++ b/tests/test_validation_output.py @@ -0,0 +1,208 @@ +"""A warning nobody can see is not a warning. + +The compiler has always collected findings into a `ValidationReport`. Nothing +displayed them on a successful run, so `orchestrator validate` printed:: + + ✓ p.yaml is valid + +for a pipeline carrying a warning that a reference could not be checked -- and +that pipeline then failed at run time on exactly that reference (#465). The +warning existed the whole time, in the log stream, which `validate` does not +write to and a script capturing stdout never sees. + +These tests pin both channels: the human one, and a structured one whose field +names are an interface rather than English prose to be parsed. +""" + +import ast +import builtins +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.contract] + +REPO = Path(__file__).resolve().parent.parent + +#: A step whose output shape is unknown to the validator, referenced by a +#: field name it therefore cannot check. Warns; does not fail. +WARNING_PIPELINE = """ +id: uo +name: UO +steps: + - id: make + tool: filesystem + action: write + parameters: + path: "./out_a.txt" + content: "A ran" + - id: use + tool: filesystem + action: write + parameters: + path: "./out_b.txt" + content: "made={{ make.path }}" +""" + +CLEAN_PIPELINE = """ +id: clean +name: Clean +steps: + - id: only + tool: filesystem + action: write + parameters: + path: "./out.txt" + content: "no references at all" +""" + + +def _cli(pipeline_text, tmp_path, *args): + pipeline = tmp_path / "p.yaml" + pipeline.write_text(pipeline_text) + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") + env["ORCHESTRATOR_AUTO_INSTALL"] = "0" + return subprocess.run( + [sys.executable, "-m", "orchestrator.cli", "validate", str(pipeline), *args], + cwd=str(tmp_path), env=env, capture_output=True, text=True, timeout=300, + ) + + +# --------------------------------------------------------------------------- +# The human channel +# --------------------------------------------------------------------------- + +@pytest.mark.e2e +def test_a_warning_appears_on_stdout(tmp_path): + """stdout, specifically. The warning was always in the log stream, and + `validate` does not write there.""" + result = _cli(WARNING_PIPELINE, tmp_path) + + assert result.returncode == 0, result.stdout + result.stderr + assert "does not declare its outputs" in result.stdout, ( + f"the warning never reached stdout:\n{result.stdout}" + ) + + +@pytest.mark.e2e +def test_the_summary_line_counts_the_warnings(tmp_path): + """`✓ is valid` alone is what let a warning pass unnoticed.""" + result = _cli(WARNING_PIPELINE, tmp_path) + first = result.stdout.splitlines()[0] + assert "warning" in first, f"the summary hides the warnings: {first!r}" + + +@pytest.mark.e2e +def test_a_finding_names_its_code_and_location(tmp_path): + """A code is what makes a finding searchable and suppressible later.""" + result = _cli(WARNING_PIPELINE, tmp_path) + assert "data_flow_undefined_output" in result.stdout + + +@pytest.mark.e2e +def test_a_clean_pipeline_says_nothing_extra(tmp_path): + """Reporting must not become noise, or it stops being read.""" + result = _cli(CLEAN_PIPELINE, tmp_path) + + assert result.returncode == 0, result.stdout + result.stderr + assert "warning" not in result.stdout.lower(), result.stdout + assert result.stdout.splitlines()[0].endswith("is valid") + + +@pytest.mark.e2e +def test_warnings_do_not_fail_validation(tmp_path): + """A warning is a warning. Making these errors would reject pipelines that + run correctly -- the false-positive class removed in #448/#450/#461.""" + assert _cli(WARNING_PIPELINE, tmp_path).returncode == 0 + + +# --------------------------------------------------------------------------- +# The structured channel +# --------------------------------------------------------------------------- + +@pytest.mark.e2e +def test_json_output_is_only_json(tmp_path): + """Anything appended after the document makes it unparseable -- which is + exactly what the catalogue report did before it was caught.""" + result = _cli(WARNING_PIPELINE, tmp_path, "--json") + payload = json.loads(result.stdout) # must not raise + assert payload["valid"] is True + + +@pytest.mark.e2e +def test_json_findings_carry_stable_fields(tmp_path): + """The field names are the interface. A consumer must not have to read the + English message to learn which step and which reference are at fault.""" + result = _cli(WARNING_PIPELINE, tmp_path, "--json") + findings = json.loads(result.stdout)["findings"] + + finding = next(f for f in findings if f["code"] == "data_flow_undefined_output") + assert finding["severity"] == "warning" + assert finding["step"] == "use" + assert finding["referenced_step"] == "make" + assert finding["referenced_field"] == "path" + assert finding["parameter_path"] + + +@pytest.mark.e2e +def test_json_reports_the_task_graph(tmp_path): + result = _cli(WARNING_PIPELINE, tmp_path, "--json") + assert set(json.loads(result.stdout)["tasks"]) == {"make", "use"} + + +@pytest.mark.e2e +def test_json_is_emitted_for_an_invalid_pipeline_too(tmp_path): + """A consumer should not have to switch parsers depending on the outcome.""" + broken = WARNING_PIPELINE.replace("{{ make.path }}", "{{ ghost.path }}") + result = _cli(broken, tmp_path, "--json") + + assert result.returncode != 0 + payload = json.loads(result.stdout) + assert payload["valid"] is False + assert "ghost" in payload["error"] + + +# --------------------------------------------------------------------------- +# The shadowed builtin that hid all of it +# --------------------------------------------------------------------------- + +def test_the_cli_module_shadows_no_builtin_at_module_scope(): + """`def list()` at module scope rebinds the builtin for the whole file. + + A click Command is callable, so a later `list(...)` did not build a list -- + it *invoked the command*, printed the configured providers and exited + before its own output. `--json` emitted nothing at all and looked like a + click registration problem. + + Anything named after a builtin here is one call away from repeating that. + """ + module = ast.parse((REPO / "src" / "orchestrator" / "cli.py").read_text()) + shadowed = sorted({ + node.name + for node in module.body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and hasattr(builtins, node.name) + }) + assert not shadowed, ( + f"these module-level names shadow builtins in cli.py: {shadowed}. " + f"Give the function its own name and pass the CLI name to the " + f"decorator, e.g. @keys.command(\"list\")." + ) + + +@pytest.mark.e2e +def test_keys_list_is_still_reachable_under_its_cli_name(tmp_path): + """Renaming the function must not rename the command.""" + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") + result = subprocess.run( + [sys.executable, "-m", "orchestrator.cli", "keys", "list"], + cwd=str(tmp_path), env=env, capture_output=True, text=True, timeout=120, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert "provider" in result.stdout.lower(), result.stdout From 27491b062c988fafa603097cfe924f69a92ead9d Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 3 Aug 2026 23:51:12 -0400 Subject: [PATCH 2/2] Ask whether the command exists, not whether the machine has keys test_keys_list_is_still_reachable ran 'orchestrator keys list', which reports the user's configured providers and exits non-zero when there are none. It passed on my machine because I have keys and failed in CI because CI has none -- by design; the default workflow is hermetic and carries no secrets. The test was asserting the wrong thing anyway. What matters is that renaming the function to list_keys did not rename the CLI command, and that is a question about registration: it is now asked of the click group directly, and an e2e companion uses --help, which resolves the command without needing any credential to exist. Verified by running the suite with the provider variables unset and HOME pointed at an empty directory, so ~/.orchestrator/.env is absent too. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_validation_output.py | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/tests/test_validation_output.py b/tests/test_validation_output.py index 62d377f..1d52a43 100644 --- a/tests/test_validation_output.py +++ b/tests/test_validation_output.py @@ -195,13 +195,35 @@ def test_the_cli_module_shadows_no_builtin_at_module_scope(): ) +def test_keys_list_is_still_registered_under_its_cli_name(): + """Renaming the function must not rename the command. + + Asked of the click group rather than by running it: `keys list` reports + the user's configured providers and exits non-zero when there are none. + Invoking it would test whether the machine has API keys, not whether the + command exists. CI has none by design -- the default workflow is + deliberately hermetic -- and the first version of this test passed on my + machine and failed there for exactly that reason. + """ + from orchestrator.cli import keys + + assert "list" in keys.commands, sorted(keys.commands) + assert keys.commands["list"].callback.__name__ == "list_keys", ( + "the command is registered, but not by the renamed function" + ) + + @pytest.mark.e2e -def test_keys_list_is_still_reachable_under_its_cli_name(tmp_path): - """Renaming the function must not rename the command.""" +def test_keys_list_is_reachable_from_the_command_line(tmp_path): + """End to end, without needing credentials to exist. + + `--help` proves the command resolves and is invocable; whether any + provider is configured is the user's business, not this test's. + """ env = dict(os.environ) env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") result = subprocess.run( - [sys.executable, "-m", "orchestrator.cli", "keys", "list"], + [sys.executable, "-m", "orchestrator.cli", "keys", "list", "--help"], cwd=str(tmp_path), env=env, capture_output=True, text=True, timeout=120, ) assert result.returncode == 0, result.stdout + result.stderr