From 3f8cf966eea1007da55e0e72fa6ab7faadcf4b81 Mon Sep 17 00:00:00 2001 From: "shipmill-romamo[bot]" <338557544+shipmill-romamo[bot]@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:24:16 +0300 Subject: [PATCH 1/2] Record a read run as ran and supersede a repeated proposal A risk: read operation has run by the time operations propose records it, yet its record stayed proposed forever, and proposing the same operation again left every older record open: one disk-full incident produced 47 records for one host. Following D-1, a read is now recorded as ran with its run's exit code (ranExitCode), and a new proposal marks any open proposal of the same operation, targets and inputs superseded, naming itself in supersededBy. decisions approve refuses both before asking for the id, and decisions list and why filter on --status (repeatable). The schema change is additive; records already on disk keep their status. Part of #26 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 + .../schemas/v1/operation-decision.schema.json | 44 ++- examples/disk-full-incident/README.md | 7 +- sdk/src/cloudfall/app.py | 61 +++- sdk/src/cloudfall/decision.py | 103 +++++- sdk/src/cloudfall/why.py | 44 ++- sdk/tests/test_decision.py | 296 +++++++++++++++++- sdk/tests/test_why.py | 69 +++- 8 files changed, 597 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 038bcf5..139ad49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,21 @@ Notable changes to Cloudfall. The format follows ### Added +- A decision record has two new statuses, added to + `operation-decision.schema.json` beside the old ones. `ran`: a `risk: read` + operation has run by the time it is recorded, so it is recorded with its + run's exit code (`ranExitCode`) instead of waiting as `proposed` forever. + `superseded`: when `operations propose` records an operation, an older + `proposed` record of the same operation, targets and inputs is closed and + names the new decision (`supersededBy`); an approved or finished record is + never touched. Since it reads the record first, `operations propose` now + stops with `RECORD_INVALID` on a record that does not load, before check + mode runs. `decisions approve` refuses a `ran` or `superseded` record with + `PRECONDITION` before asking for the decision id. `decisions list --status S` and + `cloudfall why --status S` (repeatable) keep only those statuses, and an + unknown status is refused. Records already on disk keep their status. One + disk-full incident left 47 records for `cloudfall why --host hz1`, most of + them reads and repeats (#26) - Every turn of `agent investigate` keeps its raw trace, in the stream's `turn` event (`trace`) and in the investigation record (`spec.trace`): the model's reasoning when it returns one (Nemotron's `reasoning_content`), diff --git a/config/schemas/v1/operation-decision.schema.json b/config/schemas/v1/operation-decision.schema.json index a8de463..e7f5dd4 100644 --- a/config/schemas/v1/operation-decision.schema.json +++ b/config/schemas/v1/operation-decision.schema.json @@ -192,9 +192,21 @@ "rejected", "executed", "verified", - "failed" + "failed", + "ran", + "superseded" ] }, + "ranExitCode": { + "description": "The exit code of a read operation's run, which happens when it is proposed. Set when the status is ran.", + "type": "integer", + "minimum": 0, + "maximum": 255 + }, + "supersededBy": { + "description": "The decision that proposed the same operation, targets and inputs again. Set when the status is superseded.", + "$ref": "common.schema.json#/$defs/resourceId" + }, "approval": { "type": "object", "properties": { @@ -363,6 +375,36 @@ "check", "status" ], + "allOf": [ + { + "if": { + "properties": { + "status": { + "const": "ran" + } + } + }, + "then": { + "required": [ + "ranExitCode" + ] + } + }, + { + "if": { + "properties": { + "status": { + "const": "superseded" + } + } + }, + "then": { + "required": [ + "supersededBy" + ] + } + } + ], "additionalProperties": false } }, diff --git a/examples/disk-full-incident/README.md b/examples/disk-full-incident/README.md index f1fcf0f..8fa2a61 100644 --- a/examples/disk-full-incident/README.md +++ b/examples/disk-full-incident/README.md @@ -32,8 +32,11 @@ The catalog: | `converge` | mutating | runs `site.yml` | | `postgresql-reinit` | destructive | drops the cluster with all its data and creates an empty one | -Read operations run when called and return what the hosts reported. The -others run in check mode only and leave a decision a person approves. +Read operations run when called, return what the hosts reported, and are +recorded as `ran`. The others run in check mode only and leave a decision a +person approves; proposing the same one again marks the older decision +`superseded`. `cloudfall decisions list --status proposed` shows what still +waits. ## Run it diff --git a/sdk/src/cloudfall/app.py b/sdk/src/cloudfall/app.py index 9ae77a7..da2d3dc 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, + ERROR_RECORD_INVALID, ApprovalChannel, ApprovalRequest, CheckRunner, @@ -111,6 +112,7 @@ Targets, approve, propose, + refuse_unapprovable, ) from cloudfall.domain import ( ConnectionAddress, @@ -759,6 +761,16 @@ class DecisionPayload(Payload): command = "decisions show" +@dataclass(frozen=True, slots=True, kw_only=True) +class ListDecisionsArgs(DecisionsArgs): + """Arguments of ``decisions list``.""" + + status: tuple[DecisionStatus, ...] = Flag( + default=(), + description="Only decisions with this status (repeatable)", + ) + + @dataclass(frozen=True, slots=True, kw_only=True) class ShowDecisionArgs(DecisionsArgs): """Arguments of ``decisions show``.""" @@ -803,13 +815,23 @@ def operations_show(args: ShowOperationArgs, _ctx: Ctx) -> OperationPayload: description="List the decision records this repository holds, oldest first", danger_level="safe", exit_codes=["RECORD_INVALID"], - examples=[("List the decision records", "cloudfall decisions list")], + examples=[ + ("List the decision records", "cloudfall decisions list"), + ( + "List what still waits for an approval", + "cloudfall decisions list --status proposed", + ), + ], ) -def decisions_list(args: DecisionsArgs, _ctx: Ctx) -> DecisionsPayload: +def decisions_list(args: ListDecisionsArgs, _ctx: Ctx) -> DecisionsPayload: """Answer every decision record: the record outlives the catalog.""" store = args.store() try: - documents = [decision.as_document() for decision in store.list()] + documents = [ + decision.as_document() + for decision in store.list() + if not args.status or decision.status in args.status + ] except DecisionError as error: raise _record_invalid(error) from error return DecisionsPayload( @@ -1989,7 +2011,13 @@ def _decision_failed(error: DecisionError) -> Exception: "on the fleet changes" ), danger_level="mutating", - exit_codes=["CONFIG_INVALID", "NOT_FOUND", "PRECONDITION", "CHECK_FAILED"], + exit_codes=[ + "CONFIG_INVALID", + "NOT_FOUND", + "PRECONDITION", + "CHECK_FAILED", + "RECORD_INVALID", + ], timeout=None, subprocess=Subprocess("ansible-playbook"), required_tools={"ansible-playbook": "2.21.0"}, @@ -2030,6 +2058,10 @@ def _propose(args: ProposeArgs, run: CheckRunner) -> Decided: try: decision = propose(request, store, run=run) except DecisionError as error: + # A new proposal reads the open ones it may supersede, so a broken + # record stops it the way it stops every other reader of the record. + if error.code == ERROR_RECORD_INVALID: + raise _record_invalid(error) from error raise _decision_failed(error) from error output = ( (args.root / decision.check.diff.path).read_text(encoding="utf-8") @@ -2087,6 +2119,10 @@ 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 + try: + refuse_unapprovable(decision) + except DecisionError as error: + raise _decision_failed(error) from error # Off a terminal this ends with PERSON_REQUIRED, and a mistyped id with # ATTESTATION_MISMATCH, before anything runs: --yes never approves. attestation = ctx.attest( @@ -3049,6 +3085,10 @@ class WhyArgs(DecisionsArgs): "Only decisions with a moment at or before this ISO 8601 time or date" ), ) + status: tuple[DecisionStatus, ...] = Flag( + default=(), + description="Only decisions with this status (repeatable)", + ) def __post_init__(self) -> None: """Refuse a time the question cannot be asked with.""" @@ -3065,6 +3105,7 @@ def query(self) -> WhyQuery: operation=self.operation.value if self.operation is not None else None, since=self.since, until=self.until, + status=tuple(entry.value for entry in self.status), ) @@ -3085,6 +3126,10 @@ class WhyPayload(Payload): renderers={"html": FormatRenderer(render_why_document, media_type="text/html")}, examples=[ ("Ask about one host", "cloudfall why --host h1"), + ( + "Only the decisions that verified or failed", + "cloudfall why --status verified --status failed", + ), ("One page for a person", "cloudfall why --since 2026-10-01 --format html"), ], ) @@ -3839,7 +3884,13 @@ def _provided_tools(args: McpServeArgs, ctx: Ctx) -> list[McpTool]: return _engine_tools(args, root, ctx) -_PROPOSE_EXIT_CODES = ("CONFIG_INVALID", "NOT_FOUND", "PRECONDITION", "CHECK_FAILED") +_PROPOSE_EXIT_CODES = ( + "CONFIG_INVALID", + "NOT_FOUND", + "PRECONDITION", + "CHECK_FAILED", + "RECORD_INVALID", +) def _operation_tool_schema(operation: Operation) -> dict[str, object]: diff --git a/sdk/src/cloudfall/decision.py b/sdk/src/cloudfall/decision.py index 827ffdb..23f0f7b 100644 --- a/sdk/src/cloudfall/decision.py +++ b/sdk/src/cloudfall/decision.py @@ -71,7 +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" +ERROR_RECORD_INVALID = "decision_record_invalid" class DecisionError(RuntimeError): @@ -113,6 +113,11 @@ class DecisionStatus(StrEnum): EXECUTED = "executed" VERIFIED = "verified" FAILED = "failed" + RAN = "ran" + """A read operation: it ran when it was proposed; nothing waits on it.""" + + SUPERSEDED = "superseded" + """A newer proposal of the same operation, targets and inputs replaced it.""" def gate(risk: RiskLevel) -> tuple[Requirement, str]: @@ -287,6 +292,16 @@ class Decision: execution: RunRecord | None = None verification: RunRecord | None = None verdict: str | None = None + ran_exit_code: int | None = None + superseded_by: ResourceId | None = None + + def same_call(self, other: Decision) -> bool: + """Return whether both name the same operation, targets and inputs.""" + return ( + self.operation_id == other.operation_id + and self.targets == other.targets + and _canonical(self.inputs) == _canonical(other.inputs) + ) def as_document(self) -> dict[str, object]: """Serialize the decision as a schema-valid record document.""" @@ -315,6 +330,10 @@ def as_document(self) -> dict[str, object]: spec["verify"] = self.verification.as_dict() if self.verdict is not None: spec["verdict"] = self.verdict + if self.ran_exit_code is not None: + spec["ranExitCode"] = self.ran_exit_code + if self.superseded_by is not None: + spec["supersededBy"] = self.superseded_by.value return { "apiVersion": API_VERSION, "kind": DECISION_KIND, @@ -362,7 +381,7 @@ def load(self, decision_id: ResourceId) -> Decision: ) except (UnicodeDecodeError, json.JSONDecodeError) as error: message = f"decision record is not valid JSON: {path}: {error}" - raise DecisionError(_ERROR_RECORD_INVALID, message) from error + raise DecisionError(ERROR_RECORD_INVALID, message) from error try: self.catalog.validate_named(DECISION_SCHEMA, document) except ValidationError as error: @@ -371,7 +390,7 @@ def load(self, decision_id: ResourceId) -> Decision: f"decision record does not match its schema: {path}: " f"{field}: {error.message}" ) - raise DecisionError(_ERROR_RECORD_INVALID, message) from error + raise DecisionError(ERROR_RECORD_INVALID, message) from error return _decision_from_document(document) def list(self) -> tuple[Decision, ...]: @@ -418,10 +437,19 @@ def propose( Nothing on the fleet changes: check mode is the whole point of the step. The record it leaves is what an approval is given against. + + A read operation has run by the time it is recorded, so it is recorded + as ``ran`` with its exit code rather than left waiting. An older open + proposal of the same operation, targets and inputs is marked + ``superseded`` by the new one: approving the older one would run what + the newer check already replaced. """ operation = request.operation declared = _validated_inputs(operation, request.inputs) pattern = _validated_targets(operation, request.targets) + # Read before anything runs: a record that does not load stops the + # proposal here rather than after check mode ran. + recorded = store.list() moment = (now or _utc_now)() decision_id = _decision_identifier(operation.operation_id, moment) diff_path = store.directory / f"{decision_id.value}{_DIFF_SUFFIX}" @@ -439,6 +467,7 @@ def propose( diff_path.parent.mkdir(parents=True, exist_ok=True) exit_code = (run or _run_check)(argv, environment, diff_path) requirement, reason = gate(operation.risk) + ran = requirement is Requirement.RUNS_FREELY decision = Decision( decision_id=decision_id, proposed_at=_timestamp(moment), @@ -454,9 +483,28 @@ def propose( reason=reason, basis=read_basis(request.observations, request.repository), check=_preview(exit_code, tree, diff_path, request.repository), - status=DecisionStatus.PROPOSED, + status=DecisionStatus.RAN if ran else DecisionStatus.PROPOSED, + verdict=( + f"a read operation runs when it is proposed; it exited {exit_code}" + if ran + else None + ), + ran_exit_code=exit_code if ran else None, ) store.save(decision) + for older in recorded: + if older.status is DecisionStatus.PROPOSED and older.same_call(decision): + store.update( + replace( + older, + status=DecisionStatus.SUPERSEDED, + superseded_by=decision_id, + verdict=( + f"{decision_id.value} proposed the same operation, " + "targets and inputs again" + ), + ) + ) return decision @@ -504,12 +552,7 @@ def approve( human read and the run that follows it. """ decision = request.decision - if decision.status is not DecisionStatus.PROPOSED: - message = ( - f"decision {decision.decision_id} is {decision.status.value}, so " - "there is nothing left to approve" - ) - raise DecisionError(_ERROR_NOT_PROPOSED, message) + refuse_unapprovable(decision) if not request.approver.strip(): message = "an approval records who gave it; pass --approver or set USER" raise DecisionError(_ERROR_APPROVER_UNKNOWN, message) @@ -545,6 +588,33 @@ def approve( return approved +def refuse_unapprovable(decision: Decision) -> None: + """Refuse a decision that is not an open proposal, before anything runs. + + Callers that ask a person for the decision id check this first, so + nobody types an id only to hear there was nothing to approve. + """ + if decision.status is DecisionStatus.PROPOSED: + return + if decision.status is DecisionStatus.SUPERSEDED: + message = ( + f"decision {decision.decision_id} is superseded by " + f"{decision.superseded_by}, a newer proposal of the same operation, " + "targets and inputs; approve that one instead" + ) + elif decision.status is DecisionStatus.RAN: + message = ( + f"decision {decision.decision_id} is a read operation that ran when " + "it was proposed, so there is nothing to approve" + ) + else: + message = ( + f"decision {decision.decision_id} is {decision.status.value}, so " + "there is nothing left to approve" + ) + raise DecisionError(_ERROR_NOT_PROPOSED, message) + + def _outcome( execution: RunRecord, verification: RunRecord | None, @@ -797,6 +867,11 @@ def _coerced( raise DecisionError(_ERROR_INPUT_TYPE, message) +def _canonical(inputs: Mapping[str, object]) -> str: + """Return the inputs as one comparable text: ``1`` and ``true`` differ.""" + return json.dumps(dict(inputs), sort_keys=True) + + def _decision_identifier(operation_id: ResourceId, moment: datetime) -> ResourceId: stamp = moment.strftime("%Y%m%d%H%M%S") return ResourceId.from_boundary(f"{operation_id.value}-{stamp}") @@ -887,6 +962,14 @@ def _decision_from_document(document: Mapping[str, object]) -> Decision: ), status=DecisionStatus(str(spec["status"])), verdict=str(spec["verdict"]) if "verdict" in spec else None, + ran_exit_code=( + int(cast("int", spec["ranExitCode"])) if "ranExitCode" in spec else None + ), + superseded_by=( + ResourceId.from_boundary(spec["supersededBy"]) + if "supersededBy" in spec + else None + ), execution=_run_from_document(spec.get("execution")), verification=_run_from_document(spec.get("verify")), approval=_approval_from_document(approval), diff --git a/sdk/src/cloudfall/why.py b/sdk/src/cloudfall/why.py index d3d7ce0..d6fc7f2 100644 --- a/sdk/src/cloudfall/why.py +++ b/sdk/src/cloudfall/why.py @@ -30,6 +30,7 @@ _ERROR_INSTANT_INVALID = "why_instant_invalid" _ERROR_WINDOW_INVERTED = "why_window_inverted" +_ERROR_STATUS_UNKNOWN = "why_status_unknown" class WhyError(RuntimeError): @@ -90,6 +91,7 @@ class WhyQuery: operation: ResourceId | None = None since: Instant | None = None until: Instant | None = None + statuses: frozenset[DecisionStatus] = frozenset() def __post_init__(self) -> None: """Refuse a window that ends before it starts.""" @@ -111,6 +113,7 @@ def from_boundary( operation: object = None, since: object = None, until: object = None, + status: Iterable[object] = (), ) -> WhyQuery: """Build a query from the primitives a CLI or tool call carries.""" return cls( @@ -120,6 +123,7 @@ def from_boundary( ), since=Instant.from_boundary(since) if _present(since) else None, until=Instant.from_boundary(until) if _present(until) else None, + statuses=frozenset(_status(value) for value in status), ) def as_dict(self) -> dict[str, object]: @@ -133,12 +137,16 @@ def as_dict(self) -> dict[str, object]: result["since"] = self.since.as_string() if self.until is not None: result["until"] = self.until.as_string() + if self.statuses: + result["status"] = sorted(status.value for status in self.statuses) return result def matches(self, decision: Decision) -> bool: """Return whether one decision is part of the answer.""" if self.operation is not None and decision.operation_id != self.operation: return False + if self.statuses and decision.status not in self.statuses: + return False if self.host is not None and self.host.value not in hosts_of(decision): return False if self.since is None and self.until is None: @@ -251,7 +259,7 @@ def render_why_document(document: Mapping[str, object]) -> str: '

The record holds no decision this question is about.

' ) filters = ", ".join( - f"{escape(key)} = {escape(str(value))}" + f"{escape(key)} = {escape(_filter_value(value))}" for key, value in query.items() ) asked = escape(filters) if filters else "Every decision the record holds" @@ -312,6 +320,7 @@ def _render_card(entry: Mapping[str, object]) -> str: spec = cast("Mapping[str, object]", decision["spec"]) operation = cast("Mapping[str, object]", spec["operation"]) status = DecisionStatus(str(spec["status"])) + ran_exit_code = spec.get("ranExitCode") story = cast("Sequence[str]", entry["story"]) steps = "".join(f"
  • {escape(sentence)}
    • " for sentence in story) hosts = ( @@ -323,7 +332,7 @@ def _render_card(entry: Mapping[str, object]) -> str: '
    ' f"

    {escape(str(operation['id']))}

    " f"{escape(str(entry['id']))}
    " - f'' + f'' f"{escape(status.value)}
    " f"
      {steps}
    " f'
    Hosts the record names: {hosts}
    ' @@ -331,16 +340,33 @@ def _render_card(entry: Mapping[str, object]) -> str: ) -def _tone(status: DecisionStatus) -> str: +def _tone(status: DecisionStatus, ran_exit_code: object = None) -> str: + if status is DecisionStatus.RAN: + return "healthy" if ran_exit_code == 0 else "critical" if status is DecisionStatus.VERIFIED: return "healthy" if status is DecisionStatus.FAILED: return "critical" - if status is DecisionStatus.REJECTED: + if status in {DecisionStatus.REJECTED, DecisionStatus.SUPERSEDED}: return "unknown" return "warning" +def _filter_value(value: object) -> str: + if isinstance(value, list): + return ", ".join(str(entry) for entry in cast("list[object]", value)) + return str(value) + + +def _status(value: object) -> DecisionStatus: + try: + return DecisionStatus(str(value)) + except ValueError as error: + known = ", ".join(status.value for status in DecisionStatus) + message = f"status {value!r} is not one of {known}" + raise WhyError(_ERROR_STATUS_UNKNOWN, message) from error + + def _story(decision: Decision) -> Iterable[str]: """Yield the record as sentences: saw, proposed, checked, approved, ended.""" yield _saw(decision) @@ -434,6 +460,16 @@ def _ran(subject: str, run: RunRecord, log: object) -> str: def _ended(decision: Decision) -> str: if decision.status is DecisionStatus.PROPOSED: return "It is still proposed: nobody has approved or rejected it." + if decision.status is DecisionStatus.RAN: + return ( + f"It ran when it was proposed and exited {decision.ran_exit_code}: " + "a read operation waits for no approval." + ) + if decision.status is DecisionStatus.SUPERSEDED: + return ( + f"It was superseded by {decision.superseded_by}, a newer proposal " + "of the same operation, targets and inputs; nobody approved it." + ) if decision.verdict is not None: return f"It ended {decision.status.value}: {decision.verdict}." return f"It ended {decision.status.value}." diff --git a/sdk/tests/test_decision.py b/sdk/tests/test_decision.py index 047e083..b6af7a1 100644 --- a/sdk/tests/test_decision.py +++ b/sdk/tests/test_decision.py @@ -38,6 +38,8 @@ if TYPE_CHECKING: from collections.abc import Callable, Mapping, Sequence + from cloudfall.decision import Decision + SCHEMAS = default_schema_directory() MOMENT = datetime(2026, 9, 21, 14, 30, 12, tzinfo=UTC) TERMINAL = ApprovalChannel.TERMINAL @@ -52,6 +54,9 @@ playbook: playbooks/health.yml """ +LATER = datetime(2026, 9, 21, 14, 45, 0, tzinfo=UTC) +LATEST = datetime(2026, 9, 21, 15, 0, 0, tzinfo=UTC) + def _store(repository: Path) -> DecisionStore: return DecisionStore( @@ -89,6 +94,25 @@ def _proposal( ) +def _legacy_read( + repository: Path, store: DecisionStore, moment: datetime +) -> Decision: + """Record a facts read as a version before #26 did: ``proposed``, no exit code.""" + decision = propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: moment, + _Runner(), + ) + path = repository / DECISION_DIRECTORY / f"{decision.decision_id.value}.json" + document = json.loads(path.read_text(encoding="utf-8")) + document["spec"]["status"] = "proposed" + del document["spec"]["ranExitCode"] + del document["spec"]["verdict"] + path.write_text(json.dumps(document), encoding="utf-8") + return store.load(decision.decision_id) + + class _Runner: """A runner that writes what Ansible would write, stage by stage. @@ -358,6 +382,237 @@ def test_a_read_operation_is_gated_by_nothing(tmp_path: Path) -> None: assert decision.verify_playbook is None +def test_a_read_operation_is_recorded_as_ran_with_its_exit_code( + tmp_path: Path, +) -> None: + """A read has run by the time it is recorded; it never waits (#26).""" + repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) + store = _store(repository) + + decision = propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: MOMENT, + _Runner(exit_code=2), + ) + + assert decision.status is DecisionStatus.RAN + assert decision.ran_exit_code == 2 + assert decision.check.exit_code == 2 + assert store.load(decision.decision_id) == decision + on_disk = json.loads( + (repository / DECISION_DIRECTORY / f"{decision.decision_id.value}.json") + .read_text(encoding="utf-8") + ) + assert on_disk["spec"]["status"] == "ran" + assert on_disk["spec"]["ranExitCode"] == 2 + + +def test_a_mutating_proposal_still_waits_and_has_no_run_exit_code( + tmp_path: Path, +) -> None: + repository = _repository(tmp_path) + + decision = propose( + _proposal(repository), _store(repository), lambda: MOMENT, _Runner() + ) + + assert decision.status is DecisionStatus.PROPOSED + assert decision.ran_exit_code is None + assert "ranExitCode" not in json.dumps(decision.as_document()) + + +def test_a_new_proposal_supersedes_the_open_one_of_the_same_call( + tmp_path: Path, +) -> None: + """Proposing the same thing twice leaves one open proposal, not two (#26).""" + repository = _repository(tmp_path) + store = _store(repository) + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + + second = propose(_proposal(repository), store, lambda: LATER, _Runner()) + + older = store.load(first.decision_id) + assert older.status is DecisionStatus.SUPERSEDED + assert older.superseded_by == second.decision_id + assert older.approval is None + assert older.verdict is not None + assert second.decision_id.value in older.verdict + assert store.load(second.decision_id).status is DecisionStatus.PROPOSED + on_disk = json.loads( + (repository / DECISION_DIRECTORY / f"{first.decision_id.value}.json") + .read_text(encoding="utf-8") + ) + assert on_disk["spec"]["supersededBy"] == second.decision_id.value + + +@pytest.mark.parametrize( + ("target", "inputs"), + [ + ("web-2", {"version": "1.4.0"}), + ("web-1", {"version": "1.5.0"}), + ("web-1", {"version": "1.4.0", "force": "yes"}), + ], + ids=["other-target", "other-input", "extra-input"], +) +def test_a_proposal_of_another_call_supersedes_nothing( + tmp_path: Path, target: str, inputs: dict[str, object] +) -> None: + repository = _repository(tmp_path) + store = _store(repository) + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + + propose( + _proposal(repository, target=target, inputs=inputs), + store, + lambda: LATER, + _Runner(), + ) + + assert store.load(first.decision_id).status is DecisionStatus.PROPOSED + + +def test_superseding_never_touches_a_decision_that_ran(tmp_path: Path) -> None: + """Only an open proposal is replaced: what ran stays what it was.""" + repository = _repository(tmp_path) + store = _store(repository) + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + approved = approve( + ApprovalRequest( + decision=first, approver="roman", repository=repository, via=TERMINAL + ), + store, + lambda: MOMENT, + _Runner(), + ) + + propose(_proposal(repository), store, lambda: LATER, _Runner()) + + assert store.load(first.decision_id) == approved + assert approved.status is DecisionStatus.VERIFIED + + +def test_a_read_run_supersedes_a_read_recorded_before_ran_existed( + tmp_path: Path, +) -> None: + """A record on disk that says ``proposed`` for a read still loads (#26).""" + repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) + store = _store(repository) + old = _legacy_read(repository, store, MOMENT) + assert old.status is DecisionStatus.PROPOSED + + new = propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: LATER, + _Runner(), + ) + + assert new.status is DecisionStatus.RAN + assert store.load(old.decision_id).superseded_by == new.decision_id + + +@pytest.mark.parametrize("status", ["ran", "superseded"]) +def test_approving_a_decision_that_is_not_open_is_refused( + tmp_path: Path, status: str +) -> None: + repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) + store = _store(repository) + if status == "ran": + decision = propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: MOMENT, + _Runner(), + ) + else: + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + propose(_proposal(repository), store, lambda: LATER, _Runner()) + decision = store.load(first.decision_id) + run = _Runner() + + with pytest.raises(DecisionError) as error: + approve( + ApprovalRequest( + decision=decision, + approver="roman", + repository=repository, + via=TERMINAL, + ), + store, + lambda: MOMENT, + run, + ) + + assert error.value.code == "decision_not_proposed" + assert run.recorded == [] + assert store.load(decision.decision_id) == decision + + +def test_the_issue_repro_lists_ran_superseded_and_proposed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A read, then the same mutating proposal twice (#26).""" + repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) + store = _store(repository) + propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: MOMENT, + _Runner(), + ) + propose(_proposal(repository), store, lambda: LATER, _Runner()) + propose(_proposal(repository), store, lambda: LATEST, _Runner()) + + exit_code = main(["decisions", "list", "--repository", str(repository)]) + every = json.loads(capsys.readouterr().out) + open_only = main( + [ + *("decisions", "list", "--repository", str(repository)), + *("--status", "proposed"), + ] + ) + waiting = json.loads(capsys.readouterr().out) + settled = main( + [ + *("decisions", "list", "--repository", str(repository)), + *("--status", "ran", "--status", "superseded"), + ] + ) + closed = json.loads(capsys.readouterr().out) + + assert exit_code == open_only == settled == 0 + assert [entry["spec"]["status"] for entry in every["data"]["decisions"]] == [ + "ran", + "superseded", + "proposed", + ] + assert [entry["metadata"]["id"] for entry in waiting["data"]["decisions"]] == [ + "deploy-20260921150000" + ] + assert [entry["spec"]["status"] for entry in closed["data"]["decisions"]] == [ + "ran", + "superseded", + ] + + +def test_an_unknown_status_filter_is_refused( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + repository = _repository(tmp_path) + + exit_code = main( + [ + *("decisions", "list", "--repository", str(repository)), + *("--status", "stale"), + ] + ) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 2 + assert payload["error"]["code"] == "ARG_ERROR" + + def test_the_cli_proposes_and_then_lists_the_record( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: @@ -600,17 +855,13 @@ def __call__( assert approved.verification.exit_code == 1 -def test_a_read_operation_without_verify_is_executed_not_verified( +def test_an_operation_without_verify_is_executed_not_verified( tmp_path: Path, ) -> None: + """Only a read declares no verify step, and only one recorded before #26 waits.""" repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) store = _store(repository) - proposed = propose( - _proposal(repository, "facts", target=None, inputs={}), - store, - lambda: MOMENT, - _Runner(), - ) + proposed = _legacy_read(repository, store, MOMENT) approved = approve( ApprovalRequest( @@ -776,6 +1027,30 @@ def test_an_approval_off_a_terminal_is_refused( assert store.load(proposed.decision_id).status is DecisionStatus.PROPOSED +def test_the_cli_refuses_a_superseded_decision_before_asking_for_its_id( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Nobody types an id only to hear there was nothing to approve (#26).""" + repository = _repository(tmp_path) + store = _store(repository) + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + second = propose(_proposal(repository), store, lambda: LATER, _Runner()) + + exit_code = main( + [ + *("decisions", "approve", first.decision_id.value), + *("--repository", str(repository), "--approver", "roman"), + ] + ) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 4 + assert payload["error"]["code"] == "PRECONDITION" + assert payload["error"]["context"]["code"] == "decision_not_proposed" + assert second.decision_id.value in payload["error"]["message"] + assert store.load(first.decision_id).status is DecisionStatus.SUPERSEDED + + def test_a_raw_payload_cannot_approve_off_a_terminal( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: @@ -924,12 +1199,7 @@ def test_every_outcome_says_why_in_the_record(tmp_path: Path) -> None: ) executed = approve( ApprovalRequest( - decision=propose( - _proposal(repository, "facts", target=None, inputs={}), - store, - lambda: MOMENT, - _Runner(), - ), + decision=_legacy_read(repository, store, MOMENT), approver="roman", repository=repository, via=TERMINAL, diff --git a/sdk/tests/test_why.py b/sdk/tests/test_why.py index 8977f4e..ca8fa53 100644 --- a/sdk/tests/test_why.py +++ b/sdk/tests/test_why.py @@ -29,7 +29,7 @@ def _record(repository: Path) -> tuple[Decision, Decision]: - """One verified deploy on web-1, then a proposed facts read of the fleet.""" + """One verified deploy on web-1, then a facts read of the fleet that ran.""" store = _store(repository) deploy = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) deploy = approve( @@ -168,11 +168,66 @@ def test_every_sentence_of_the_story_cites_the_record(tmp_path: Path) -> None: "It ended verified: the run succeeded and the verify step changed nothing." ) - proposed = read.explanations[0].story - assert facts.status is DecisionStatus.PROPOSED - assert "was proposed against the whole fleet with no inputs." in proposed[1] - assert proposed[3].startswith("Nothing gated it:") - assert proposed[-1] == ("It is still proposed: nobody has approved or rejected it.") + ran = read.explanations[0].story + assert facts.status is DecisionStatus.RAN + assert "was proposed against the whole fleet with no inputs." in ran[1] + assert ran[3].startswith("Nothing gated it:") + assert ran[-1] == ( + "It ran when it was proposed and exited 0: a read operation waits for " + "no approval." + ) + + +def test_a_superseded_proposal_says_what_replaced_it(tmp_path: Path) -> None: + repository = _repository(tmp_path) + store = _store(repository) + first = propose(_proposal(repository), store, lambda: MOMENT, _Runner()) + second = propose(_proposal(repository), store, lambda: LATER, _Runner()) + + told = answer(store, WhyQuery.from_boundary(status=["superseded"])) + + assert [e.decision.decision_id for e in told.explanations] == [first.decision_id] + assert told.explanations[0].story[-1] == ( + f"It was superseded by {second.decision_id.value}, a newer proposal of " + "the same operation, targets and inputs; nobody approved it." + ) + + +def test_a_status_question_is_answered( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """``--status`` is repeatable and keeps only the statuses asked for (#26).""" + repository = _repository(tmp_path) + _record(repository) + + exit_code = main( + [ + *("why", "--repository", str(repository)), + *("--status", "ran", "--status", "proposed"), + ] + ) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 0 + assert payload["data"]["query"] == {"status": ["proposed", "ran"]} + assert [entry["id"] for entry in payload["data"]["answers"]] == [ + "facts-20260922080000" + ] + + +def test_an_unknown_status_question_is_refused( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + repository = _repository(tmp_path) + + exit_code = main(["why", "--repository", str(repository), "--status", "stale"]) + + payload = json.loads(capsys.readouterr().out) + assert exit_code == 2 + assert payload["error"]["code"] == "ARG_ERROR" + with pytest.raises(WhyError) as error: + WhyQuery.from_boundary(status=["stale"]) + assert error.value.code == "why_status_unknown" def test_the_cli_answers_in_json_without_a_catalog( @@ -217,7 +272,7 @@ def test_the_cli_answers_as_a_page( assert "Cloudfall Why" in page assert "2 decisions" in page assert 'class="badge healthy">verified' in page - assert 'class="badge warning">proposed' in page + assert 'class="badge healthy">ran' in page assert "roman approved it at 2026-09-21T14:30:12Z (via terminal)" in page From 5b6ca302be592e86ae40c2137e108bd4804c8b4e Mon Sep 17 00:00:00 2001 From: "shipmill-romamo[bot]" <338557544+shipmill-romamo[bot]@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:39:11 +0300 Subject: [PATCH 2/2] Do not tell an operator to approve a read that already ran A read record written before #26 still says proposed. The next run of the same read supersedes it with a decision recorded as ran, and approving the old one pointed at that ran decision with "approve that one instead". Co-Authored-By: Claude Opus 5.5 --- sdk/src/cloudfall/decision.py | 9 ++++++++- sdk/tests/test_decision.py | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/sdk/src/cloudfall/decision.py b/sdk/src/cloudfall/decision.py index 23f0f7b..7cf28d4 100644 --- a/sdk/src/cloudfall/decision.py +++ b/sdk/src/cloudfall/decision.py @@ -597,10 +597,17 @@ def refuse_unapprovable(decision: Decision) -> None: if decision.status is DecisionStatus.PROPOSED: return if decision.status is DecisionStatus.SUPERSEDED: + # A read is recorded as ran, so the decision that replaced an old + # read record has nothing left to approve either. + instead = ( + "it ran when it was proposed, so there is nothing to approve" + if decision.requirement is Requirement.RUNS_FREELY + else "approve that one instead" + ) message = ( f"decision {decision.decision_id} is superseded by " f"{decision.superseded_by}, a newer proposal of the same operation, " - "targets and inputs; approve that one instead" + f"targets and inputs; {instead}" ) elif decision.status is DecisionStatus.RAN: message = ( diff --git a/sdk/tests/test_decision.py b/sdk/tests/test_decision.py index b6af7a1..6d5ebec 100644 --- a/sdk/tests/test_decision.py +++ b/sdk/tests/test_decision.py @@ -27,6 +27,7 @@ gate, propose, read_basis, + refuse_unapprovable, run_command, ) from cloudfall.domain import ResourceId @@ -512,6 +513,29 @@ def test_a_read_run_supersedes_a_read_recorded_before_ran_existed( assert store.load(old.decision_id).superseded_by == new.decision_id +def test_a_superseded_read_does_not_point_at_a_run_to_approve( + tmp_path: Path, +) -> None: + """The read that replaced it ran already; there is nothing to approve.""" + repository = _repository(tmp_path, deploy=DEPLOY, facts=FACTS) + store = _store(repository) + old = _legacy_read(repository, store, MOMENT) + new = propose( + _proposal(repository, "facts", target=None, inputs={}), + store, + lambda: LATER, + _Runner(), + ) + + with pytest.raises(DecisionError) as error: + refuse_unapprovable(store.load(old.decision_id)) + + assert error.value.code == "decision_not_proposed" + assert new.decision_id.value in error.value.detail + assert "approve that one" not in error.value.detail + assert "ran when it was proposed" in error.value.detail + + @pytest.mark.parametrize("status", ["ran", "superseded"]) def test_approving_a_decision_that_is_not_open_is_refused( tmp_path: Path, status: str