diff --git a/CHANGELOG.md b/CHANGELOG.md index 4110861..36cfbb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -451,6 +451,14 @@ Notable changes to Cloudfall. The format follows stopped by a signal or a timeout keeps its own exit code, and one stopped while it waits on the model still leaves no record; `--no-stream` is unchanged (#44) +- `agent investigate` exits 4 `PRECONDITION` (code + `agent_investigations_unwritable`), naming the investigations folder, + when it cannot write the investigation record, such as when a file + stands where the folder should be. A run that ended crashed with a + traceback instead; one whose reader closed stdout mid-run printed the + traceback and exited 0. Such a stopped run still exits non-zero, now + `PRECONDITION` with the reason as a JSON line on stderr, though its + record is lost (#56) - The operator proposal schema refuses `approval.via` on an autonomous (policy) approval. Such a receipt used to pass validation and lose the field on load; it now fails schema validation like any other invalid diff --git a/sdk/src/cloudfall/app.py b/sdk/src/cloudfall/app.py index 234d4e1..a6df8d8 100644 --- a/sdk/src/cloudfall/app.py +++ b/sdk/src/cloudfall/app.py @@ -2221,6 +2221,14 @@ def decisions_approve(args: ApproveDecisionArgs, ctx: Ctx) -> Decided: ``INVESTIGATION_STOPPED`` instead. A stream closed by a signal or a timeout keeps the exit code treaty gives it. """ +stream_unrecorded: list[InvestigationError] = [] +"""Why an ``agent investigate`` stream closed mid-run could not record its run. + +Raised from the stream's close, the error would reach only treaty, which +prints its traceback and still exits 0; kept here instead, ``cloudfall.cli`` +reports it on stderr and exits ``PRECONDITION``, as a run whose record could +not be written does when its stream ran to the end. +""" @dataclass(frozen=True, slots=True, kw_only=True) @@ -2424,7 +2432,12 @@ def agent_investigate( if stopped is None: message = "the investigation ended before its reader went away" raise RuntimeError(message) from None - _save_investigation(store, stopped) + try: + store.save(stopped) + except InvestigationError as error: + # The record is lost; the run still ends non-zero, PRECONDITION. + stream_unrecorded.append(error) + raise GeneratorExit from error stream_stopped.set() raise investigation = _save_investigation(store, investigation) diff --git a/sdk/src/cloudfall/cli.py b/sdk/src/cloudfall/cli.py index 45d69a3..7681587 100644 --- a/sdk/src/cloudfall/cli.py +++ b/sdk/src/cloudfall/cli.py @@ -2,10 +2,18 @@ from __future__ import annotations +import json import sys from typing import TYPE_CHECKING -from cloudfall.app import INVESTIGATION_STOPPED, app, stream_stopped +from treaty import FrameworkCode + +from cloudfall.app import ( + INVESTIGATION_STOPPED, + app, + stream_stopped, + stream_unrecorded, +) if TYPE_CHECKING: from collections.abc import Sequence @@ -16,25 +24,47 @@ def main(argv: Sequence[str] | None = None) -> int: """Run one command in-process and return its exit code.""" - stream_stopped.clear() + _forget_streams() return _stopped(app.run(list(sys.argv[1:] if argv is None else argv))) def run() -> None: """Installed console-script entry point.""" - stream_stopped.clear() + _forget_streams() try: app.main() except SystemExit as exiting: # app.main() exits with treaty's code; only a stopped stream's changes. code = exiting.code - if isinstance(code, int) and _stopped(code) != code: - raise SystemExit(INVESTIGATION_STOPPED) from None + if isinstance(code, int): + stopped = _stopped(code) + if stopped != code: + raise SystemExit(stopped) from None raise +def _forget_streams() -> None: + stream_stopped.clear() + stream_unrecorded.clear() + + def _stopped(code: int) -> int: - """Return INVESTIGATION_STOPPED when the reader left an investigation mid-run.""" - if stream_stopped.is_set() and code in OUTPUT_CLOSED: + """Return the exit code of an investigation whose reader left it mid-run. + + INVESTIGATION_STOPPED when its record was kept; PRECONDITION, with the + reason on stderr, when the record could not be written. + """ + if code not in OUTPUT_CLOSED: + return code + if stream_unrecorded: + error = stream_unrecorded[-1] + line = { + "code": FrameworkCode.PRECONDITION.name, + "message": error.detail, + "context": {"code": error.code}, + } + sys.stderr.write(f"{json.dumps(line)}\n") + return int(FrameworkCode.PRECONDITION) + if stream_stopped.is_set(): return INVESTIGATION_STOPPED return code diff --git a/sdk/src/cloudfall/investigation.py b/sdk/src/cloudfall/investigation.py index 639b14c..296091d 100644 --- a/sdk/src/cloudfall/investigation.py +++ b/sdk/src/cloudfall/investigation.py @@ -56,6 +56,7 @@ ERROR_MODEL_REFUSED = "agent_model_refused" ERROR_MODEL_UNREACHABLE = "agent_model_unreachable" ERROR_INVESTIGATION_EXISTS = "agent_investigation_exists" +ERROR_INVESTIGATIONS_UNWRITABLE = "agent_investigations_unwritable" SYSTEM_PROMPT = """\ You are the on-call agent for a server fleet. You act only through the tools, \ @@ -321,9 +322,14 @@ def save(self, investigation: Investigation) -> Investigation: """Persist a new investigation and return it as saved. An investigation started in the same second as a recorded one gets a - numeric suffix on its id; no record is ever overwritten. + numeric suffix on its id; no record is ever overwritten. A directory + that cannot be written, such as a file in its place, raises + ``InvestigationError`` naming it. """ - self.directory.mkdir(parents=True, exist_ok=True) + try: + self.directory.mkdir(parents=True, exist_ok=True) + except OSError as error: + raise self._unwritable(error) from error base = investigation.investigation_id.value for attempt in range(1, SAVE_ATTEMPTS + 1): identifier = base if attempt == 1 else f"{base}-{attempt}" @@ -338,10 +344,19 @@ def save(self, investigation: Investigation) -> Investigation: record.write(f"{json.dumps(document, indent=2, sort_keys=True)}\n") except FileExistsError: continue + except OSError as error: + raise self._unwritable(error) from error return saved message = f"{SAVE_ATTEMPTS} investigations already exist for {base}" raise InvestigationError(ERROR_INVESTIGATION_EXISTS, message) + def _unwritable(self, error: OSError) -> InvestigationError: + reason = error.strerror or type(error).__name__ + message = ( + f"cannot write the investigation record to {self.directory}: {reason}" + ) + return InvestigationError(ERROR_INVESTIGATIONS_UNWRITABLE, message) + @dataclass(frozen=True, slots=True) class ToolCallText: diff --git a/sdk/tests/test_investigation.py b/sdk/tests/test_investigation.py index 50caa1b..c39b520 100644 --- a/sdk/tests/test_investigation.py +++ b/sdk/tests/test_investigation.py @@ -15,6 +15,7 @@ from cloudfall.app import app from cloudfall.cli import main from cloudfall.investigation import ( + ERROR_INVESTIGATIONS_UNWRITABLE, ERROR_MODEL_REFUSED, TRACE_TEXT_LIMIT, AgentTool, @@ -1191,10 +1192,13 @@ def log_message(self, *_args: object) -> None: return -def test_a_reader_closing_stdout_mid_run_leaves_the_record_stopped( - tmp_path: Path, -) -> None: - repository = _repository(tmp_path) +def _closed_mid_run( + tmp_path: Path, repository: Path +) -> tuple[dict[str, Any], dict[str, Any], int, str]: + """Run the CLI, read the first turn and its call, then close stdout. + + Return the two events read, the exit code and what went to stderr. + """ (repository / "playbooks" / "facts.yml").write_text( "---\n- hosts: localhost\n connection: local\n gather_facts: false\n" " tasks:\n - name: Report\n ansible.builtin.debug:\n" @@ -1246,13 +1250,21 @@ def test_a_reader_closing_stdout_mid_run_leaves_the_record_stopped( finally: server.shutdown() server.server_close() + return first, call, exit_code, stderr_path.read_text(encoding="utf-8") + + +def test_a_reader_closing_stdout_mid_run_leaves_the_record_stopped( + tmp_path: Path, +) -> None: + repository = _repository(tmp_path) + + first, call, exit_code, err = _closed_mid_run(tmp_path, repository) assert (first["kind"], call["kind"]) == ("turn", "call") assert exit_code == 94 # Nothing more is written once stdout is gone: no traceback, and no # "Exception ignored ... BrokenPipeError" at shutdown either. stderr holds # only the stream's JSON warning lines, such as UNTRUSTED_CONTENT. - err = stderr_path.read_text(encoding="utf-8") assert "BrokenPipe" not in err assert "Traceback" not in err assert all("code" in json.loads(line) for line in err.splitlines()) @@ -1265,3 +1277,67 @@ def test_a_reader_closing_stdout_mid_run_leaves_the_record_stopped( [step] = spec["steps"] assert step["outcome"] == "ran" assert list((repository / "decisions").glob(f"{step['decision']}*")) + + +def test_an_unwritable_investigations_folder_is_refused_naming_it( + tmp_path: Path, +) -> None: + report, rotate = _tools() + done = investigate( + "alert", + _tool_list(report, rotate), + ENDPOINT, + _Script(_say("ROOT CAUSE: disk\nPROPOSED: none\nWHY: read only.")), + ) + folder = tmp_path / "investigations" + folder.write_text("not a folder\n", encoding="utf-8") + store = InvestigationStore( + directory=folder, catalog=SchemaCatalog(default_schema_directory()) + ) + + with pytest.raises(InvestigationError) as error: + store.save(done) + + assert error.value.code == ERROR_INVESTIGATIONS_UNWRITABLE + assert str(folder) in error.value.detail + + +def test_a_record_that_cannot_be_written_exits_precondition( + tmp_path: Path, capsys: pytest.CaptureFixture[str], model_url: str +) -> None: + repository = _repository(tmp_path) + (repository / "investigations").write_text("not a folder\n", encoding="utf-8") + key = tmp_path / "model.key" + key.write_text("sk-test\n", encoding="utf-8") + _Model.replies[:] = [ + _say("ROOT CAUSE: /var/log/shop\nPROPOSED: none\nWHY: read only.") + ] + + exit_code = _investigate_against(repository, key, model_url) + + out = capsys.readouterr() + payload = _streamed(out.out) + assert exit_code == 4, payload + assert payload["error"]["code"] == "PRECONDITION" + assert payload["error"]["context"]["code"] == ERROR_INVESTIGATIONS_UNWRITABLE + assert str(repository / "investigations") in payload["error"]["message"] + assert "Traceback" not in out.err + + +def test_a_stopped_run_whose_record_cannot_be_written_exits_precondition( + tmp_path: Path, +) -> None: + repository = _repository(tmp_path) + (repository / "investigations").write_text("not a folder\n", encoding="utf-8") + + first, call, exit_code, err = _closed_mid_run(tmp_path, repository) + + assert (first["kind"], call["kind"]) == ("turn", "call") + # Not 0, as when the save crashed, and not 94: no stopped record was kept. + assert exit_code == 4, err + assert "Traceback" not in err + lines = [json.loads(line) for line in err.splitlines()] + [refused] = [line for line in lines if line["code"] == "PRECONDITION"] + assert refused["context"]["code"] == ERROR_INVESTIGATIONS_UNWRITABLE + assert str(repository / "investigations") in refused["message"] + assert (repository / "investigations").is_file()