From 327ef40eacfa04352ca255b1d61edf7dce30ce04 Mon Sep 17 00:00:00 2001 From: "shipmill-romamo[bot]" <338557544+shipmill-romamo[bot]@users.noreply.github.com> Date: Thu, 8 Oct 2026 21:10:28 +0300 Subject: [PATCH] Require a person at a terminal to approve a decision Bump treaty to 1.0.0rc38 for requires_person and ctx.attest. decisions approve now asks the person to type the decision id at a terminal; no flag answers it, so an agent's shell call exits 4 with PERSON_REQUIRED and a mistyped id with ATTESTATION_MISMATCH. The record keeps how the approval arrived as approval.via (terminal; unknown for older records), and cloudfall why tells it. treaty reserves --yes on a requires_person command, so the old Flag(confirm=True) preview without --yes is gone; decisions show previews. This departs from the decided design and waits on the maintainer. The treaty bump also changes the stream line shape of dashboard serve and operator run, and mcp serve's signal exit codes. Fixes #25 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 24 ++ .../schemas/v1/operation-decision.schema.json | 8 + examples/disk-full-incident/README.md | 10 +- pyproject.toml | 2 +- sdk/README.md | 4 + sdk/src/cloudfall/app.py | 56 ++--- sdk/src/cloudfall/commands.py | 9 +- sdk/src/cloudfall/decision.py | 65 ++++-- sdk/src/cloudfall/why.py | 19 +- sdk/tests/test_agent_server.py | 3 +- sdk/tests/test_app.py | 6 +- sdk/tests/test_commands.py | 1 - sdk/tests/test_decision.py | 217 ++++++++++++++++-- sdk/tests/test_why.py | 47 +++- uv.lock | 8 +- 15 files changed, 389 insertions(+), 90 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9957ca..b8d61ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ Notable changes to Cloudfall. The format follows ### Added +- A decision record's approval says how it arrived, as `approval.via`: + `terminal` when a person typed the decision id at a terminal. A record + written before reads as `via: unknown`. `cloudfall why` tells it in the + approval sentence. The field is optional in + `operation-decision.schema.json` (#25) - `agent investigate --replay DIR` plays a real run back instead of running Ansible: DIR is the decisions directory of a run on real hosts, and each check-mode call the model makes answers with the output and per-host @@ -130,6 +135,12 @@ Notable changes to Cloudfall. The format follows ### Changed +- treaty 1.0.0rc38 instead of 1.0.0rc29. `dashboard serve` and + `operator run` write each event in `json` and `jsonl` as a bare JSON line + with `_seq` instead of an envelope with `data` and `meta.seq`, and end + with one `"_summary": true` line or the error envelope. `mcp serve` + stopped by `SIGINT` or `SIGTERM` exits 130 or 143 with the `CANCELLED` + envelope instead of 0, and writes no envelope when it stops cleanly (#25) - The package is classified as Beta on PyPI instead of Alpha - The site's landing page is redesigned for technical founders leaving the cloud and for teams running servers for clients: a light, illustrated @@ -420,6 +431,19 @@ Notable changes to Cloudfall. The format follows `failed` receipt, left the proposal `proposed` with no trace that it ran, and showed the caller a traceback +### Security + +- `cloudfall decisions approve ` asks the person to type the decision id + at a terminal before it runs anything, and no flag answers that, `--yes` + included. Off a terminal, as from an agent's shell or a script, it exits 4 + with `PERSON_REQUIRED` and runs nothing; a mistyped id exits 4 with + `ATTESTATION_MISMATCH`. Before, `--yes` ran it headless and `--approver` + fell back to `$USER`, so an agent could approve its own proposal in the + person's name. Without `--yes` the command no longer previews the + proposal: `decisions show` does. Scripted or CI approvals stop working. + This is a speed bump, not a boundary: a process running as the same OS + user can fake a terminal (#25) + ## [0.5.1] - 2026-09-22 ### Changed diff --git a/config/schemas/v1/operation-decision.schema.json b/config/schemas/v1/operation-decision.schema.json index 04102ea..a8de463 100644 --- a/config/schemas/v1/operation-decision.schema.json +++ b/config/schemas/v1/operation-decision.schema.json @@ -206,6 +206,14 @@ "approvedAt": { "type": "string", "format": "date-time" + }, + "via": { + "description": "How the approval arrived: terminal when a person typed the decision id at a terminal. A record written before this field reads as unknown.", + "type": "string", + "enum": [ + "terminal", + "unknown" + ] } }, "required": [ diff --git a/examples/disk-full-incident/README.md b/examples/disk-full-incident/README.md index f278a6d..f1fcf0f 100644 --- a/examples/disk-full-incident/README.md +++ b/examples/disk-full-incident/README.md @@ -61,12 +61,14 @@ cloudfall agent investigate \ ``` The answer names the root cause and the decisions it recorded, in the order -to approve them. Nothing on the host has changed yet. Review and approve: +to approve them. Nothing on the host has changed yet. Review, then approve +at your terminal, where each approval asks you to type the decision id (an +agent's shell call is refused with `PERSON_REQUIRED`): ```console -cloudfall decisions approve shop-logrotate- # shows the proposal -cloudfall decisions approve shop-logrotate- --yes # runs it, then verifies -cloudfall decisions approve postgresql-restart- --yes +cloudfall decisions show shop-logrotate- # shows the proposal +cloudfall decisions approve shop-logrotate- # type the id; runs it, then verifies +cloudfall decisions approve postgresql-restart- cloudfall why --host hz1 --format html > why.html ``` diff --git a/pyproject.toml b/pyproject.toml index 8e70b0e..7c156de 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,7 +21,7 @@ dependencies = [ "ansible-core>=2.21,<2.22", "jsonschema[format]>=4.23,<5", "pyyaml>=6,<7", - "treaty==1.0.0rc29", + "treaty==1.0.0rc38", ] [project.urls] diff --git a/sdk/README.md b/sdk/README.md index a212bd2..644c573 100644 --- a/sdk/README.md +++ b/sdk/README.md @@ -93,6 +93,10 @@ project and never overwrite an existing resource. `cloudfall init`, `decisions approve`, `operator run` and `dashboard serve` are never tools: the server starts inside an existing project, approving a recorded operation is a person's command, and the loops run until stopped. +`decisions approve` asks the person to type the decision id at a terminal; +off a terminal it exits 4 with `PERSON_REQUIRED`, `--yes` or not. That is a +speed bump against an agent with a shell, not a boundary: a process running +as the same OS user can fake a terminal. ```console uv run cloudfall mcp serve --project config/examples diff --git a/sdk/src/cloudfall/app.py b/sdk/src/cloudfall/app.py index ddbf60f..5ac8378 100644 --- a/sdk/src/cloudfall/app.py +++ b/sdk/src/cloudfall/app.py @@ -100,6 +100,7 @@ CHECK_TIMEOUT_SECONDS, DECISION_DIRECTORY, ERROR_DECISION_MISSING, + ApprovalChannel, ApprovalRequest, CheckRunner, DecisionError, @@ -1957,14 +1958,6 @@ class ApproveDecisionArgs(DecisionsArgs): default=None, description="Who is approving (default: the USER environment variable)", ) - yes: bool = Flag( - default=False, - confirm=True, - description=( - "Change the servers; without it the command shows the recorded " - "proposal and runs nothing" - ), - ) @dataclass(frozen=True, slots=True) @@ -2058,11 +2051,14 @@ def _propose(args: ProposeArgs, run: CheckRunner) -> Decided: @decisions.command( "approve", - # A person approves; no agent tool stands in for it. + # A person approves; no agent tool stands in for it. The person types the + # decision id at a terminal, which no flag answers, --yes included. + # requires_person keeps it off MCP too; mcp=False says so here as well. mcp=False, + requires_person=True, description=( - "Approve one recorded proposal, run it, and verify it; without --yes it " - "shows the proposal and runs nothing" + "Approve one recorded proposal, run it, and verify it; a person types " + "the decision id at a terminal, and `decisions show` previews it" ), danger_level="mutating", exit_codes=["NOT_FOUND", "PRECONDITION", "RECORD_INVALID", "NOT_VERIFIED"], @@ -2071,12 +2067,14 @@ def _propose(args: ProposeArgs, run: CheckRunner) -> Decided: subprocess=Subprocess("ansible-playbook"), required_tools={"ansible-playbook": "2.21.0"}, examples=[ - ("Review a proposal", "cloudfall decisions approve restart-nginx-20260101"), - ("Run it", "cloudfall decisions approve restart-nginx-20260101 --yes"), + ( + "Approve and run a proposal, typing its id at a terminal", + "cloudfall decisions approve restart-nginx-20260101", + ), ], ) def decisions_approve(args: ApproveDecisionArgs, ctx: Ctx) -> Decided: - """Run what was proposed, as recorded, and record how it ended.""" + """Run what was proposed, as recorded, once a person typed its id.""" store = DecisionStore( directory=args.root / args.decisions, catalog=SchemaCatalog(args.schemas) ) @@ -2086,21 +2084,22 @@ def decisions_approve(args: ApproveDecisionArgs, ctx: Ctx) -> Decided: if error.code == ERROR_DECISION_MISSING: raise Exit.NOT_FOUND(error.detail, context={"code": error.code}) from error raise _record_invalid(error) from error - if not args.yes: - # Without --yes the run is treaty's dry run (Flag(confirm=True)). - return Decided( - effect="would_update", - status="pending", - decision=decision.as_document(), - next=[ - f"review the recorded diff at {decision.check.diff.path}", - "approve with --yes to run it", - ], - ) + # Off a terminal this ends with PERSON_REQUIRED, and a mistyped id with + # ATTESTATION_MISMATCH, before anything runs: --yes never approves. + attestation = ctx.attest( + f"The diff is at {decision.check.diff.path}. Type the decision id to " + f"approve {decision.decision_id.value} and run it", + expected=decision.decision_id.value, + ) approver = args.approver if args.approver is not None else ctx.env.get("USER", "") try: approved = approve( - ApprovalRequest(decision=decision, approver=approver, repository=args.root), + ApprovalRequest( + decision=decision, + approver=approver, + repository=args.root, + via=ApprovalChannel(attestation.channel), + ), store, run=_check_runner(ctx, args.root), ) @@ -3817,7 +3816,10 @@ def propose_operation(arguments: Mapping[str, object], ctx: Ctx) -> Decided: if operation.risk is RiskLevel.READ: return decided record = cast("Mapping[str, Mapping[str, object]]", decided.decision) - approve = f"{APPROVAL_COMMAND} {record['metadata']['id']} --yes" + approve = ( + "a person approves it at a terminal, typing the decision id: " + f"{APPROVAL_COMMAND} {record['metadata']['id']}" + ) return dataclasses.replace(decided, next=[approve]) return McpTool( diff --git a/sdk/src/cloudfall/commands.py b/sdk/src/cloudfall/commands.py index dcdfe94..6a7142a 100644 --- a/sdk/src/cloudfall/commands.py +++ b/sdk/src/cloudfall/commands.py @@ -267,11 +267,12 @@ def _plan(subject: str, *extra: str) -> OutputShape: "decisions approve", CommandEffect.SERVERS, "approve one recorded proposal, run it for real, and verify it", - gate=_YES, - output=( - _shape("status", "decision", "next", when="without `--yes`"), - _shape("status", "decision", when="with `--yes`"), + gate=( + "a person types the decision id at a terminal, which no flag answers " + "(`--yes` included); off a terminal it exits 4 with `PERSON_REQUIRED`, " + "and `decisions show` previews the proposal" ), + output=(_shape("status", "decision"),), ), CommandContract( "cloudfall", diff --git a/sdk/src/cloudfall/decision.py b/sdk/src/cloudfall/decision.py index c24410a..d292846 100644 --- a/sdk/src/cloudfall/decision.py +++ b/sdk/src/cloudfall/decision.py @@ -68,6 +68,7 @@ ERROR_DECISION_MISSING = "decision_missing" _ERROR_NOT_PROPOSED = "decision_not_proposed" _ERROR_APPROVER_UNKNOWN = "decision_approver_unknown" +_ERROR_CHANNEL_UNKNOWN = "decision_approval_channel_unknown" class DecisionError(RuntimeError): @@ -223,16 +224,43 @@ def as_dict(self) -> dict[str, object]: } +class ApprovalChannel(StrEnum): + """How an approval reached the record. + + A person typing the decision id back at a terminal is the only channel + an approval is given through. A record written before the channel was + recorded says nothing about it, and reads as unknown. + """ + + TERMINAL = "terminal" + """A person typed the decision id at a terminal.""" + + UNKNOWN = "unknown" + """The record predates the channel; it cannot say how the approval came.""" + + @classmethod + def from_record(cls, value: object) -> ApprovalChannel: + """Read the channel a record names; a record that names none is unknown.""" + if value is None: + return cls.UNKNOWN + return cls(str(value)) + + @dataclass(frozen=True, slots=True) class Approval: - """Who let one operation through, and when.""" + """Who let one operation through, when, and how it arrived.""" approver: str approved_at: str + via: ApprovalChannel def as_dict(self) -> dict[str, object]: """Serialize the approval for the record.""" - return {"approver": self.approver, "approvedAt": self.approved_at} + return { + "approver": self.approver, + "approvedAt": self.approved_at, + "via": self.via.value, + } @dataclass(frozen=True, slots=True) @@ -435,6 +463,7 @@ class ApprovalRequest: decision: Decision approver: str repository: Path + via: ApprovalChannel @dataclass(frozen=True, slots=True) @@ -469,6 +498,12 @@ def approve( if not request.approver.strip(): message = "an approval records who gave it; pass --approver or set USER" raise DecisionError(_ERROR_APPROVER_UNKNOWN, message) + if request.via is ApprovalChannel.UNKNOWN: + message = ( + "an approval records how it arrived; a new one cannot be of an " + "unknown channel" + ) + raise DecisionError(_ERROR_CHANNEL_UNKNOWN, message) clock = now or _utc_now stage = _Stage(request=request, store=store, runner=run or _run_check) moment = clock() @@ -482,7 +517,9 @@ def approve( approved = replace( decision, approval=Approval( - approver=request.approver.strip(), approved_at=_timestamp(moment) + approver=request.approver.strip(), + approved_at=_timestamp(moment), + via=request.via, ), execution=execution, verification=verification, @@ -837,14 +874,16 @@ def _decision_from_document(document: Mapping[str, object]) -> Decision: verdict=str(spec["verdict"]) if "verdict" in spec else None, execution=_run_from_document(spec.get("execution")), verification=_run_from_document(spec.get("verify")), - approval=( - Approval( - approver=str(cast("Mapping[str, object]", approval)["approver"]), - approved_at=str( - cast("Mapping[str, object]", approval)["approvedAt"] - ), - ) - if isinstance(approval, dict) - else None - ), + approval=_approval_from_document(approval), + ) + + +def _approval_from_document(approval: object) -> Approval | None: + if not isinstance(approval, dict): + return None + record = cast("Mapping[str, object]", approval) + return Approval( + approver=str(record["approver"]), + approved_at=str(record["approvedAt"]), + via=ApprovalChannel.from_record(record.get("via")), ) diff --git a/sdk/src/cloudfall/why.py b/sdk/src/cloudfall/why.py index f1da1a9..d3d7ce0 100644 --- a/sdk/src/cloudfall/why.py +++ b/sdk/src/cloudfall/why.py @@ -20,13 +20,13 @@ from html import escape from typing import TYPE_CHECKING, cast -from cloudfall.decision import DecisionStatus, Requirement +from cloudfall.decision import ApprovalChannel, DecisionStatus, Requirement from cloudfall.domain import Hostname, ResourceId if TYPE_CHECKING: from collections.abc import Iterable, Mapping, Sequence - from cloudfall.decision import Decision, DecisionStore, RunRecord + from cloudfall.decision import Approval, Decision, DecisionStore, RunRecord _ERROR_INSTANT_INVALID = "why_instant_invalid" _ERROR_WINDOW_INVERTED = "why_window_inverted" @@ -348,10 +348,7 @@ def _story(decision: Decision) -> Iterable[str]: yield _checked(decision) yield _gated(decision) if decision.approval is not None: - yield ( - f"{decision.approval.approver} approved it at " - f"{decision.approval.approved_at}." - ) + yield _approved(decision.approval) if decision.execution is not None: yield _ran("The run", decision.execution, decision.execution.log.path) if decision.verification is not None: @@ -361,6 +358,16 @@ def _story(decision: Decision) -> Iterable[str]: yield _ended(decision) +def _approved(approval: Approval) -> str: + said = ( + f"{approval.approver} approved it at {approval.approved_at} " + f"(via {approval.via.value})" + ) + if approval.via is ApprovalChannel.UNKNOWN: + return f"{said}: the record predates noting how an approval arrived." + return f"{said}: a person typed its id at a terminal." + + def _saw(decision: Decision) -> str: observed = decision.basis.observed_at if not observed: diff --git a/sdk/tests/test_agent_server.py b/sdk/tests/test_agent_server.py index 4a9f04f..4a76c9d 100644 --- a/sdk/tests/test_agent_server.py +++ b/sdk/tests/test_agent_server.py @@ -207,7 +207,8 @@ def test_an_operation_tool_records_a_proposal_and_changes_nothing( assert decision["spec"]["status"] == "proposed" assert decision["spec"]["inputs"] == {"version": "2.0.0"} assert proposed["data"]["next"] == [ - f"cloudfall decisions approve {decision['metadata']['id']} --yes" + "a person approves it at a terminal, typing the decision id: " + f"cloudfall decisions approve {decision['metadata']['id']}" ] assert not (repository / "marker.txt").exists() recorded = decisions["data"]["decisions"] diff --git a/sdk/tests/test_app.py b/sdk/tests/test_app.py index 00225c6..b176734 100644 --- a/sdk/tests/test_app.py +++ b/sdk/tests/test_app.py @@ -118,7 +118,7 @@ def test_dashboard_serve_streams_where_it_listens_until_stopped( readable, _, _ = select.select([process.stdout], [], [], 30) assert readable, "the listening event never arrived" listening = json.loads(process.stdout.readline()) - url = listening["data"]["dashboard"]["url"] + url = listening["dashboard"]["url"] with urllib.request.urlopen(f"{url}operations.json", timeout=10) as response: # noqa: S310 - local server assert response.status == 200 process.send_signal(signal.SIGTERM) @@ -128,8 +128,8 @@ def test_dashboard_serve_streams_where_it_listens_until_stopped( process.kill() process.wait() - assert listening["meta"]["seq"] == 1 - assert listening["data"]["status"] == "ok" + assert listening["_seq"] == 1 + assert listening["status"] == "ok" assert closing["error"]["code"] == "CANCELLED" diff --git a/sdk/tests/test_commands.py b/sdk/tests/test_commands.py index c91f7d5..95bdacb 100644 --- a/sdk/tests/test_commands.py +++ b/sdk/tests/test_commands.py @@ -65,7 +65,6 @@ def test_every_yes_gated_command_is_classified_as_changing_servers() -> None: "restart", "data copy", "migrate", - "decisions approve", } assert gated <= changing for command in commands_with_effect(CommandEffect.SERVERS): diff --git a/sdk/tests/test_decision.py b/sdk/tests/test_decision.py index bc508d3..9a227f6 100644 --- a/sdk/tests/test_decision.py +++ b/sdk/tests/test_decision.py @@ -2,16 +2,19 @@ from __future__ import annotations +import io import json from datetime import UTC, datetime from pathlib import Path from typing import TYPE_CHECKING import pytest +from cloudfall.app import app from cloudfall.catalog import RiskLevel, TargetScope, load_catalog from cloudfall.cli import main from cloudfall.decision import ( DECISION_DIRECTORY, + ApprovalChannel, ApprovalRequest, DecisionError, DecisionStatus, @@ -30,12 +33,14 @@ from cloudfall.resources import default_schema_directory from cloudfall.validation import SchemaCatalog from test_catalog import DEPLOY, FACTS, _repository +from treaty import CommandPath if TYPE_CHECKING: from collections.abc import Mapping, Sequence SCHEMAS = default_schema_directory() MOMENT = datetime(2026, 9, 21, 14, 30, 12, tzinfo=UTC) +TERMINAL = ApprovalChannel.TERMINAL WIPE = """\ id: wipe @@ -499,7 +504,7 @@ def test_an_approval_runs_the_recorded_proposal_and_verifies_it( approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -530,7 +535,7 @@ def test_an_approved_run_is_not_check_mode(tmp_path: Path) -> None: approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -550,7 +555,7 @@ def test_a_failed_run_is_recorded_as_failed_and_skips_verify( approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -581,7 +586,7 @@ def __call__( approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -609,7 +614,7 @@ def test_a_read_operation_without_verify_is_executed_not_verified( approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -624,14 +629,14 @@ def test_one_proposal_is_approved_once(tmp_path: Path) -> None: store = _store(repository) proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) request = ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ) approved = approve(request, store, lambda: MOMENT, _Runner()) with pytest.raises(DecisionError) as error: approve( ApprovalRequest( - decision=approved, approver="roman", repository=repository + decision=approved, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -648,7 +653,9 @@ def test_an_approval_records_who_gave_it(tmp_path: Path) -> None: with pytest.raises(DecisionError) as error: approve( - ApprovalRequest(decision=proposed, approver=" ", repository=repository), + ApprovalRequest( + decision=proposed, approver=" ", repository=repository, via=TERMINAL + ), store, lambda: MOMENT, _Runner(), @@ -657,30 +664,194 @@ def test_an_approval_records_who_gave_it(tmp_path: Path) -> None: assert error.value.code == "decision_approver_unknown" -def test_the_cli_shows_the_proposal_before_it_runs_anything( +class _Terminal(io.StringIO): + """A stream that says it is a terminal, as a person's shell is.""" + + def isatty(self) -> bool: + return True + + +LOCAL_PLAY = ( + "---\n- hosts: localhost\n connection: local\n gather_facts: false\n" + " tasks:\n - name: Nothing\n ansible.builtin.debug:\n" + " msg: nothing\n" +) + + +def _proposed_locally(repository: Path, capsys: pytest.CaptureFixture[str]) -> str: + """Propose deploy against localhost through the CLI; return the decision id.""" + _observations(repository, "web-1") + for name in ("deploy.yml", "health.yml"): + (repository / "playbooks" / name).write_text(LOCAL_PLAY, encoding="utf-8") + exit_code = main( + [ + *("operations", "propose", "deploy"), + *("--repository", str(repository), "--target", "localhost"), + *("--input", "version=1.4.0"), + ] + ) + proposed = json.loads(capsys.readouterr().out) + assert exit_code == 0 + return str(proposed["data"]["decision"]["metadata"]["id"]) + + +def _approve_at_terminal( + repository: Path, decision_id: str, typed: str +) -> tuple[int, dict[str, object]]: + """Run ``decisions approve`` as a person at a terminal who types ``typed``.""" + stdout = _Terminal() + exit_code = app.run( + [ + *("decisions", "approve", decision_id), + *("--repository", str(repository), "--approver", "roman"), + *("--format", "json"), + ], + stdin=_Terminal(f"{typed}\n"), + stdout=stdout, + stderr=io.StringIO(), + ) + return exit_code, json.loads(stdout.getvalue()) + + +def test_a_person_at_a_terminal_approves_by_typing_the_decision_id( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + repository = _repository(tmp_path) + decision_id = _proposed_locally(repository, capsys) + + exit_code, payload = _approve_at_terminal(repository, decision_id, decision_id) + + assert exit_code == 0, payload + loaded = _store(repository).load(ResourceId.from_boundary(decision_id)) + assert loaded.status is DecisionStatus.VERIFIED + assert loaded.approval is not None + assert loaded.approval.approver == "roman" + assert loaded.approval.via is ApprovalChannel.TERMINAL + on_disk = json.loads( + (repository / DECISION_DIRECTORY / f"{decision_id}.json").read_text( + encoding="utf-8" + ) + ) + assert on_disk["spec"]["approval"]["via"] == "terminal" + + +def test_a_mistyped_decision_id_approves_nothing( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - """Without --yes the approval command changes nothing.""" + repository = _repository(tmp_path) + decision_id = _proposed_locally(repository, capsys) + + exit_code, payload = _approve_at_terminal(repository, decision_id, "yes") + + error = payload["error"] + assert isinstance(error, dict) + assert exit_code == 4 + assert error["code"] == "ATTESTATION_MISMATCH" + loaded = _store(repository).load(ResourceId.from_boundary(decision_id)) + assert loaded.status is DecisionStatus.PROPOSED + assert loaded.approval is None + + +@pytest.mark.parametrize("flags", [["--yes"], []], ids=["yes", "bare"]) +def test_an_approval_off_a_terminal_is_refused( + tmp_path: Path, capsys: pytest.CaptureFixture[str], flags: list[str] +) -> None: + """An agent's shell call cannot approve, ``--yes`` or not (#25).""" repository = _repository(tmp_path) store = _store(repository) proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) exit_code = main( [ - "decisions", - "approve", - proposed.decision_id.value, - "--repository", - str(repository), + *("decisions", "approve", proposed.decision_id.value), + *("--repository", str(repository), "--approver", "roman"), + *flags, ] ) payload = json.loads(capsys.readouterr().out) - assert exit_code == 0 - assert payload["data"]["status"] == "pending" - assert payload["data"]["effect"] == "would_update" - assert payload["meta"]["dry_run"] is True - assert payload["data"]["decision"]["spec"]["status"] == "proposed" + assert exit_code == 4 + assert payload["error"]["code"] == "PERSON_REQUIRED" + assert payload["error"]["retryable"] is False + assert store.load(proposed.decision_id).status is DecisionStatus.PROPOSED + + +def test_a_raw_payload_cannot_approve_off_a_terminal( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + repository = _repository(tmp_path) + store = _store(repository) + proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + payload_in = { + "decision": proposed.decision_id.value, + "repository": str(repository), + "approver": "roman", + "yes": True, + } + + exit_code = main( + ["decisions", "approve", "--raw-payload", json.dumps(payload_in)] + ) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 4 + assert payload["error"]["code"] == "PERSON_REQUIRED" + assert store.load(proposed.decision_id).status is DecisionStatus.PROPOSED + + +def test_the_approval_command_is_a_persons_and_never_a_tool() -> None: + command = app.commands[CommandPath("decisions.approve")] + + assert command.requires_person is True + assert command.mcp is False + + +def test_a_record_written_before_the_channel_reads_as_unknown( + tmp_path: Path, +) -> None: + repository = _repository(tmp_path) + store = _store(repository) + proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + approved = approve( + ApprovalRequest( + decision=proposed, approver="roman", repository=repository, via=TERMINAL + ), + store, + lambda: MOMENT, + _Runner(), + ) + path = repository / DECISION_DIRECTORY / f"{approved.decision_id.value}.json" + document = json.loads(path.read_text(encoding="utf-8")) + del document["spec"]["approval"]["via"] + path.write_text(json.dumps(document), encoding="utf-8") + + loaded = store.load(approved.decision_id) + + assert loaded.approval is not None + assert loaded.approval.via is ApprovalChannel.UNKNOWN + shown = json.loads(json.dumps(loaded.as_document())) + assert shown["spec"]["approval"]["via"] == "unknown" + + +def test_a_new_approval_names_how_it_arrived(tmp_path: Path) -> None: + repository = _repository(tmp_path) + store = _store(repository) + proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + + with pytest.raises(DecisionError) as error: + approve( + ApprovalRequest( + decision=proposed, + approver="roman", + repository=repository, + via=ApprovalChannel.UNKNOWN, + ), + store, + lambda: MOMENT, + _Runner(), + ) + + assert error.value.code == "decision_approval_channel_unknown" assert store.load(proposed.decision_id).status is DecisionStatus.PROPOSED @@ -693,7 +864,7 @@ def test_an_approved_decision_round_trips_through_the_store( proposed = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -719,7 +890,7 @@ def test_a_verify_step_that_changes_the_fleet_did_not_verify_it( approved = approve( ApprovalRequest( - decision=proposed, approver="roman", repository=repository + decision=proposed, approver="roman", repository=repository, via=TERMINAL ), store, lambda: MOMENT, @@ -745,6 +916,7 @@ def test_every_outcome_says_why_in_the_record(tmp_path: Path) -> None: ), approver="roman", repository=repository, + via=TERMINAL, ), store, lambda: MOMENT, @@ -760,6 +932,7 @@ def test_every_outcome_says_why_in_the_record(tmp_path: Path) -> None: ), approver="roman", repository=repository, + via=TERMINAL, ), store, lambda: MOMENT, diff --git a/sdk/tests/test_why.py b/sdk/tests/test_why.py index 5b75630..8977f4e 100644 --- a/sdk/tests/test_why.py +++ b/sdk/tests/test_why.py @@ -8,7 +8,14 @@ import pytest from cloudfall.cli import main -from cloudfall.decision import ApprovalRequest, DecisionStatus, approve, propose +from cloudfall.decision import ( + DECISION_DIRECTORY, + ApprovalChannel, + ApprovalRequest, + DecisionStatus, + approve, + propose, +) from cloudfall.why import Instant, WhyError, WhyQuery, answer, render_why_html from test_catalog import _repository from test_decision import MOMENT, _proposal, _Runner, _store @@ -26,7 +33,12 @@ def _record(repository: Path) -> tuple[Decision, Decision]: store = _store(repository) deploy = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) deploy = approve( - ApprovalRequest(decision=deploy, approver="roman", repository=repository), + ApprovalRequest( + decision=deploy, + approver="roman", + repository=repository, + via=ApprovalChannel.TERMINAL, + ), store, lambda: MOMENT, _Runner(), @@ -142,7 +154,10 @@ def test_every_sentence_of_the_story_cites_the_record(tmp_path: Path) -> None: assert story[2].startswith("Check mode would have changed web-1 and left 1 host") assert deploy.check.diff.sha256[:12] in story[2] assert story[3].startswith("It waited for an approval:") - assert story[4] == "roman approved it at 2026-09-21T14:30:12Z." + assert story[4] == ( + "roman approved it at 2026-09-21T14:30:12Z (via terminal): a person " + "typed its id at a terminal." + ) assert story[5].startswith( "The run at 2026-09-21T14:30:12Z exited 0 and changed web-1" ) @@ -180,6 +195,7 @@ def test_the_cli_answers_in_json_without_a_catalog( assert entry["id"] == "deploy-20260921143012" assert entry["hosts"] == ["web-1", "web-2"] assert entry["decision"]["spec"]["approval"]["approver"] == "roman" + assert entry["decision"]["spec"]["approval"]["via"] == "terminal" assert entry["decision"]["spec"]["verify"]["changed"] == [] assert entry["decision"]["spec"]["check"]["diff"]["sha256"] assert entry["decision"]["spec"]["basis"]["observedAt"] == { @@ -202,7 +218,30 @@ def test_the_cli_answers_as_a_page( assert "2 decisions" in page assert 'class="badge healthy">verified' in page assert 'class="badge warning">proposed' in page - assert "roman approved it at 2026-09-21T14:30:12Z." in page + assert "roman approved it at 2026-09-21T14:30:12Z (via terminal)" in page + + +def test_an_approval_recorded_before_the_channel_says_via_unknown( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + repository = _repository(tmp_path) + deploy, _ = _record(repository) + path = repository / DECISION_DIRECTORY / f"{deploy.decision_id.value}.json" + document = json.loads(path.read_text(encoding="utf-8")) + del document["spec"]["approval"]["via"] + path.write_text(json.dumps(document), encoding="utf-8") + + told = answer(_store(repository), WhyQuery.from_boundary(operation="deploy")) + exit_code = main(["why", "--repository", str(repository), "--host", "web-1"]) + + assert told.explanations[0].story[4] == ( + "roman approved it at 2026-09-21T14:30:12Z (via unknown): the record " + "predates noting how an approval arrived." + ) + payload = json.loads(capsys.readouterr().out) + assert exit_code == 0 + entry = payload["data"]["answers"][0] + assert entry["decision"]["spec"]["approval"]["via"] == "unknown" def test_the_cli_reports_a_bad_window_as_json( diff --git a/uv.lock b/uv.lock index 060090b..35ea2b9 100644 --- a/uv.lock +++ b/uv.lock @@ -285,7 +285,7 @@ requires-dist = [ { name = "jsonschema", extras = ["format"], specifier = ">=4.23,<5" }, { name = "mcp", marker = "extra == 'mcp'", specifier = ">=2,<3" }, { name = "pyyaml", specifier = ">=6,<7" }, - { name = "treaty", specifier = "==1.0.0rc29" }, + { name = "treaty", specifier = "==1.0.0rc38" }, ] provides-extras = ["mcp"] @@ -1085,11 +1085,11 @@ wheels = [ [[package]] name = "treaty" -version = "1.0.0rc29" +version = "1.0.0rc38" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/ec/50/e564b3e75431d5df24f170cf8726968a536bceb0ff7468da2308afac92de/treaty-1.0.0rc29.tar.gz", hash = "sha256:11f30f51f051d7da2008a2b15970edcb708a7d9becae8f1a0ecdd9631bb70a2f", size = 562870, upload-time = "2026-10-03T10:03:37.738Z" } +sdist = { url = "https://files.pythonhosted.org/packages/e8/b9/e20c42a64380f69eeff8ccb8aa04ca28dda5f8d00505f074757f20c0eba7/treaty-1.0.0rc38.tar.gz", hash = "sha256:580437e2d0f70f13340d5201639d9fba0ffec853980f9819fb0d365f82b76927", size = 515401, upload-time = "2026-10-08T17:50:35.324Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/02/b0/80f2a44a6151ddbc295f769dbf63542c88548f170f90fabb8c16d338e618/treaty-1.0.0rc29-py3-none-any.whl", hash = "sha256:1c7314db7ee78a7dc16199444c6486ece85a4ed1b24b622aedccf23b152d73c4", size = 561162, upload-time = "2026-10-03T10:03:35.85Z" }, + { url = "https://files.pythonhosted.org/packages/75/ee/c53221910616676275225137eaaa890d500f4d8ed7f82a0428059b994c56/treaty-1.0.0rc38-py3-none-any.whl", hash = "sha256:670573b0ed1b311531170abd2f3dc74f9443b757c1e60cf64ad4617a2d765770", size = 565262, upload-time = "2026-10-08T17:50:33.528Z" }, ] [[package]]