diff --git a/CHANGELOG.md b/CHANGELOG.md index b8d61ee..6459408 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -392,6 +392,12 @@ Notable changes to Cloudfall. The format follows ### Fixed +- A decision record that is not JSON or does not match its schema answers + `RECORD_INVALID` (exit 86, code `decision_record_invalid`) naming the file + and the failing field, from `decisions show`, `decisions list`, + `decisions approve` and `why`; it used to crash them with exit 1 and no + envelope. `decisions list` and `why` still stop at the first bad record + rather than skip it (#41) - `migrate --env-file NAME=PATH` and `--data NAME=PATH` resolve a relative path against the project again, as every other path does; since the move to treaty they resolved against the current directory diff --git a/sdk/src/cloudfall/decision.py b/sdk/src/cloudfall/decision.py index d292846..827ffdb 100644 --- a/sdk/src/cloudfall/decision.py +++ b/sdk/src/cloudfall/decision.py @@ -27,6 +27,8 @@ from pathlib import Path from typing import TYPE_CHECKING, cast +from jsonschema.exceptions import ValidationError + from cloudfall.catalog import RiskLevel, TargetScope from cloudfall.domain import ResourceId @@ -69,6 +71,7 @@ _ERROR_NOT_PROPOSED = "decision_not_proposed" _ERROR_APPROVER_UNKNOWN = "decision_approver_unknown" _ERROR_CHANNEL_UNKNOWN = "decision_approval_channel_unknown" +_ERROR_RECORD_INVALID = "decision_record_invalid" class DecisionError(RuntimeError): @@ -353,10 +356,22 @@ def load(self, decision_id: ResourceId) -> Decision: if not path.is_file(): message = f"decision does not exist: {path}" raise DecisionError(ERROR_DECISION_MISSING, message) - document = cast( - "dict[str, object]", json.loads(path.read_text(encoding="utf-8")) - ) - self.catalog.validate_named(DECISION_SCHEMA, document) + try: + document = cast( + "dict[str, object]", json.loads(path.read_text(encoding="utf-8")) + ) + except (UnicodeDecodeError, json.JSONDecodeError) as error: + message = f"decision record is not valid JSON: {path}: {error}" + raise DecisionError(_ERROR_RECORD_INVALID, message) from error + try: + self.catalog.validate_named(DECISION_SCHEMA, document) + except ValidationError as error: + field = ".".join(str(part) for part in error.absolute_path) or "(root)" + message = ( + f"decision record does not match its schema: {path}: " + f"{field}: {error.message}" + ) + raise DecisionError(_ERROR_RECORD_INVALID, message) from error return _decision_from_document(document) def list(self) -> tuple[Decision, ...]: diff --git a/sdk/tests/test_decision.py b/sdk/tests/test_decision.py index 9a227f6..047e083 100644 --- a/sdk/tests/test_decision.py +++ b/sdk/tests/test_decision.py @@ -36,7 +36,7 @@ from treaty import CommandPath if TYPE_CHECKING: - from collections.abc import Mapping, Sequence + from collections.abc import Callable, Mapping, Sequence SCHEMAS = default_schema_directory() MOMENT = datetime(2026, 9, 21, 14, 30, 12, tzinfo=UTC) @@ -943,3 +943,95 @@ def test_every_outcome_says_why_in_the_record(tmp_path: Path) -> None: assert executed.verdict == ( "the run succeeded; the operation declares no verify step" ) + + +def _not_json(path: Path) -> str: + path.write_text("{not json", encoding="utf-8") + return "not valid JSON" + + +def _bad_status(path: Path) -> str: + document = json.loads(path.read_text(encoding="utf-8")) + document["spec"]["status"] = "bogus" + path.write_text(json.dumps(document), encoding="utf-8") + return "spec.status" + + +def _bad_via(path: Path) -> str: + document = json.loads(path.read_text(encoding="utf-8")) + document["spec"]["approval"]["via"] = "pigeon" + path.write_text(json.dumps(document), encoding="utf-8") + return "spec.approval.via" + + +RECORD_COMMANDS: dict[str, tuple[str, ...]] = { + "show": ("decisions", "show", "{id}"), + "list": ("decisions", "list"), + "approve": ("decisions", "approve", "{id}", "--approver", "roman"), + "why": ("why",), +} + + +def _run_on_record( + command: str, repository: Path, decision_id: str +) -> tuple[int, dict[str, object]]: + """Run one record-reading command as a person at a terminal.""" + argv = [part.replace("{id}", decision_id) for part in RECORD_COMMANDS[command]] + stdout = _Terminal() + exit_code = app.run( + [*argv, "--repository", str(repository), "--format", "json"], + stdin=_Terminal(f"{decision_id}\n"), + stdout=stdout, + stderr=io.StringIO(), + ) + return exit_code, json.loads(stdout.getvalue()) + + +@pytest.mark.parametrize("command", sorted(RECORD_COMMANDS)) +@pytest.mark.parametrize( + "corrupt", [_not_json, _bad_status, _bad_via], ids=["json", "status", "via"] +) +def test_an_invalid_record_answers_record_invalid_naming_the_file( + tmp_path: Path, command: str, corrupt: Callable[[Path], str] +) -> None: + """A broken record is a typed RECORD_INVALID, never a crash (#41).""" + repository = _repository(tmp_path) + store = _store(repository) + proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + approve( + ApprovalRequest( + decision=proposed, approver="roman", repository=repository, via=TERMINAL + ), + store, + lambda: MOMENT, + _Runner(), + ) + path = repository / DECISION_DIRECTORY / f"{proposed.decision_id.value}.json" + named = corrupt(path) + + exit_code, payload = _run_on_record(command, repository, proposed.decision_id.value) + + error = payload["error"] + assert isinstance(error, dict) + assert exit_code == 86, payload + assert error["code"] == "RECORD_INVALID" + assert error["context"]["code"] == "decision_record_invalid" + assert str(path) in error["message"] + assert named in error["message"] + + +def test_a_store_refuses_an_invalid_record_with_a_decision_error( + tmp_path: Path, +) -> None: + repository = _repository(tmp_path) + store = _store(repository) + proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + path = repository / DECISION_DIRECTORY / f"{proposed.decision_id.value}.json" + _bad_status(path) + + with pytest.raises(DecisionError) as error: + store.list() + + assert error.value.code == "decision_record_invalid" + assert str(path) in error.value.detail + assert "spec.status" in error.value.detail