diff --git a/CLAUDE.md b/CLAUDE.md index 70b35fe..733705d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1092,18 +1092,12 @@ raised anywhere inside astra's validation or resolution was reported as a bad target — a rerun misdiagnosing itself, at the one place nobody is watching. -**Known violation, unfixed: a malformed `astra.yaml` tracebacks out of -`lc status` and `lc materialize --check`** as a raw -`yaml.scanner.ScannerError`. `plan._validate` is the gate that turns a -bad spec into ASTRA's own errors, but it only catches what *validation* -reports — a spec that does not parse raises inside -`validate_analysis_schema` → `astra.helpers.load_yaml` before there is -anything to validate. (`lc materialize` usually masks it by refusing a -dirty tree first, which is not a fix.) The shape of the fix is a -`yaml.YAMLError` catch in `_validate` naming the file and the line, the -same way the validator's own errors are rendered. Pre-existing and -independent of the one-output-one-file change; written down here because -it contradicts the invariant directly above it. +**Malformed YAML is a validation refusal.** `plan._validate` catches +`yaml.YAMLError` from ASTRA's validators and includes the parser's own +diagnostic in the existing `ProjectError`, preserving the source file, +line and column where available. The same gate covers `astra.yaml` and +universe files, so `lc status` and `lc materialize --check` report the +fault cleanly. Normal `lc materialize` still refuses a dirty tree first. **`git_sha` in a manifest is the commit the run *started* at**, not the commit the run went on to create. It is the code that produced the output. diff --git a/src/lightcone/engine/plan.py b/src/lightcone/engine/plan.py index 513f410..bfab68d 100644 --- a/src/lightcone/engine/plan.py +++ b/src/lightcone/engine/plan.py @@ -251,13 +251,18 @@ def _validate(spec_path: Path, universes: list[Path]) -> None: validate_analysis_schema, validate_universe_file, ) + from yaml import YAMLError - problems = [ - *validate_analysis_schema(spec_path), - *(str(e) for e in validate_analysis_file(spec_path)), - ] - for path in universes: - problems += [f"{path.name}: {e}" for e in validate_universe_file(path, spec_path)] + try: + problems = [ + *validate_analysis_schema(spec_path), + *(str(e) for e in validate_analysis_file(spec_path)), + ] + for path in universes: + problems += [f"{path.name}: {e}" for e in validate_universe_file(path, spec_path)] + except YAMLError as e: + # Parser diagnostics already name the source file and its location. + problems = [str(e)] if problems: listed = "\n".join(f" {problem}" for problem in problems) raise ProjectError( diff --git a/tests/test_cli_yaml.py b/tests/test_cli_yaml.py new file mode 100644 index 0000000..1ea2049 --- /dev/null +++ b/tests/test_cli_yaml.py @@ -0,0 +1,59 @@ +"""Malformed YAML through the real CLI, with no engine or tool stubs.""" + +from __future__ import annotations + +import os +import subprocess +import sys +from collections.abc import Callable +from pathlib import Path + +import pytest + +_SPEC = '''version: "0.1" +name: demo +inputs: [] +outputs: + - id: fit + type: data + format: txt + recipe: + command: echo valid > {output} +decisions: {} +''' + + +@pytest.mark.parametrize("argv", [["status"], ["materialize", "--check"]]) +@pytest.mark.parametrize("json_output", [False, True]) +@pytest.mark.parametrize("filename", ["astra.yaml", "universes/baseline.yaml"]) +def test_malformed_yaml_is_reported_without_a_traceback_or_writes( + analysis: Callable[..., Path], argv: list[str], json_output: bool, filename: str, +) -> None: + root = analysis(_SPEC) + command = [sys.executable, "-c", "from lightcone.cli import main; main()", *argv] + if json_output: + command.append("--json") + env = dict(os.environ, PATH=f"{Path(sys.executable).parent}{os.pathsep}{os.environ['PATH']}") + + valid = subprocess.run(command, cwd=root, env=env, capture_output=True, text=True) + assert valid.returncode == (0 if argv == ["status"] else 1), valid.stderr + assert valid.stderr == "" + assert "baseline/fit" in valid.stdout + assert not (root / "results/baseline/fit.txt").exists() + + malformed = root / filename + malformed.write_text("name: a: b\n") + before_paths = set(root.rglob("*")) + before = {p.relative_to(root): p.read_bytes() for p in root.rglob("*") if p.is_file()} + + result = subprocess.run(command, cwd=root, env=env, capture_output=True, text=True) + + assert result.returncode == 1 + assert result.stdout == "" + assert result.stderr.startswith("Error: ") + assert str(malformed) in result.stderr + assert "mapping values are not allowed here" in result.stderr + assert "line 1, column 8" in result.stderr + assert "Traceback" not in result.stderr + assert set(root.rglob("*")) == before_paths + assert {p.relative_to(root): p.read_bytes() for p in root.rglob("*") if p.is_file()} == before diff --git a/tests/test_plan.py b/tests/test_plan.py index 9f069d3..48b1af4 100644 --- a/tests/test_plan.py +++ b/tests/test_plan.py @@ -293,6 +293,55 @@ def test_no_universe_is_a_clean_error(tmp_path: Path) -> None: _build(tmp_path) +@pytest.mark.parametrize(("filename", "content", "diagnostic"), [ + ("astra.yaml", "name: a: b\n", "line 1, column 8"), + ("universes/baseline.yaml", "id: a: b\n", "line 1, column 6"), + ("astra.yaml", "outputs: [\n", "line 2, column 1"), + ("astra.yaml", "name: \x00\n", "unacceptable character #x0000"), +]) +def test_malformed_yaml_is_a_clean_validation_error( + tmp_path: Path, filename: str, content: str, diagnostic: str, +) -> None: + root = _project(tmp_path) + path = root / filename + path.write_text(content) + + with pytest.raises(ProjectError, match="does not validate") as raised: + _build(root) + + assert str(path) in str(raised.value) + assert diagnostic in str(raised.value) + + +def test_malformed_external_analysis_names_its_own_file(tmp_path: Path) -> None: + root = _project(tmp_path, _SPEC + '\nanalyses:\n child:\n path: child\n') + child = root / "child" / "astra.yaml" + child.parent.mkdir() + child.write_text("name: a: b\n") + + with pytest.raises(ProjectError, match="does not validate") as raised: + _build(root) + + assert str(child) in str(raised.value) + assert "line 1, column 8" in str(raised.value) + + +def test_validation_does_not_hide_unrelated_errors( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, +) -> None: + from astra import validation + + error = ValueError("unexpected validator defect") + + def broken(path: Path) -> list[str]: + raise error + + monkeypatch.setattr(validation, "validate_analysis_schema", broken) + with pytest.raises(ValueError) as raised: + _build(_project(tmp_path)) + assert raised.value is error + + def test_an_output_without_a_format_is_refused_by_name(tmp_path: Path) -> None: """lc names the file from it, so there is nowhere to write the output. Every offender at once: a spec is fixed in one pass, not one run per