From 341588383442079a406f700e174111edd50cdb5b Mon Sep 17 00:00:00 2001 From: arpan Date: Fri, 11 Sep 2026 23:42:14 +0530 Subject: [PATCH 1/3] The attempt ceiling: one human yes is no longer unlimited dispatches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC-v0.1 §5.4's `FAILED` row permits a renewal and bounds nothing, so an executor that raises `NotExecuted` on every call is an unlimited number of provider dispatches, each one individually correct. SPEC-v0.7 §5 bounds it where the operator asks, and this carries the amendment into SPEC-v0.1 §5.4 itself, beneath the unchanged table. `max_attempts`, an action-entry policy key, an integer of at least 1, counting the attempts that may execute on one effect key with the first included. It needs `schema: ctrlrun.policy/v5`, a superset of v4 as v4 is of v3; 0, a negative, a bool, a float, a string and a mapping are each a `PolicyError` at load naming the key, the action and the line. Absent means no ceiling, which is 0.6.1's behaviour exactly, and there is no default. The decision is taken on the attempt number the store assigned, after the reservation and before the executor, because two callers that both read attempt N-1 would both pass a read taken before reserving. Above the ceiling the executor is not called, the record is released FAILED with an error naming the ceiling, `EFFECT_RESERVATION_REFUSED` carries reason `attempt_ceiling` with the attempt and the ceiling, a `blocked` receipt is written, and `ActionDenied(reason="attempt_ceiling")` is raised. A read before the approval gate refuses the ordinary sequential case earlier, writing nothing, spending no presented approval and creating no approval request; it refuses only a FAILED record and is never the guarantee, so it gets its own test and the check gets its own deterministic window. Verify gains G15, graded through the reconcile route where only the check can refuse. G5 now selects only an action whose ceiling permits a renewal and is N/A where the ceiling is the only reason it cannot, because G5's control is a renewal and `max_attempts: 1` would otherwise report a correct kernel as a failure. No new error type, no new event type, no new StateStore method, no new Control method, no CLI change. --- .github/workflows/ci.yml | 7 +- CHANGELOG.md | 28 + docs/SPEC-v0.1.md | 9 + docs/SPEC-v0.7.md | 66 ++ src/ctrlrun/control.py | 170 ++++ src/ctrlrun/policy.py | 169 +++- src/ctrlrun/receipt.py | 9 +- src/ctrlrun/verify/guarantees.py | 37 + src/ctrlrun/verify/scenarios.py | 210 ++++- tests/test_attempt_cap.py | 1340 ++++++++++++++++++++++++++++++ tests/test_verify.py | 14 +- tests/test_verify_action.py | 13 +- tests/test_verify_report.py | 13 +- 13 files changed, 2046 insertions(+), 39 deletions(-) create mode 100644 tests/test_attempt_cap.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b3cc24..ed7ab05 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -122,10 +122,11 @@ jobs: echo "templates: $TEMPLATES ($TEMPLATES_NA not applicable)" test "$AUTHORITY" = "verified 11/11" # G13 is N/A on SQLite, the action's default store: SQLite has no clock of its own - # to diverge from (SPEC-v0.7 §8.9). - test "$AUTHORITY_NA" = "1" + # to diverge from; G15 is N/A because neither document declares `max_attempts` + # (SPEC-v0.7 §8.9). + test "$AUTHORITY_NA" = "2" test "$TEMPLATES" = "verified 6/6" - test "$TEMPLATES_NA" = "6" + test "$TEMPLATES_NA" = "7" test -s verify-badge.json test -s verify-report.json test -s verify-report.xml diff --git a/CHANGELOG.md b/CHANGELOG.md index b82b49e..d3f9d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,34 @@ any change to one appears here. Postgres `--store-url` and `N/A` on SQLite, and the catalogue moves to `ctrlrun.guarantees/v3`; the store conformance suite gains a `clock` case, `not_applicable` on SQLite and the in-memory store because neither has a clock of its own. +- **The attempt ceiling, `max_attempts`** (SPEC-v0.7 §5, item 4, and the amendment to + `docs/SPEC-v0.1.md` §5.4). A new action-entry policy key, an integer of at least 1, bounding the + attempts that may **execute** on one effect key, the first included: `max_attempts: 3` is the + first attempt and two renewals. It needs `schema: ctrlrun.policy/v5`, a new schema version that + is a superset of `v4` as `v4` is of `v3`; `0`, a negative, a `bool`, a float, a string and a + mapping are each a `PolicyError` at load, naming the key, the action and the line. The ceiling + is inside the policy hash, so a receipt records which one refused an attempt. + **The decision is taken on the attempt number the store assigned**, after the reservation and + before the executor, because two callers that both read attempt *N−1* would both pass a read + taken before reserving. Above the ceiling the executor is not called, the record is released as + `FAILED` with an error naming the ceiling, `EFFECT_RESERVATION_REFUSED` carries + `reason: "attempt_ceiling"` with the attempt and the ceiling, a `blocked` receipt is written, + and `ActionDenied(reason="attempt_ceiling")` is raised. A read of the record before the approval + gate refuses the ordinary sequential case earlier, writing nothing, spending no presented + approval and creating no approval request; it refuses only a `FAILED` record and is never the + guarantee. In observe mode the refusal is recorded as `would_have.blocked_reason: + "attempt_ceiling"` and the action runs. Verify gains **G15**, and G5 now selects only an action + whose ceiling permits a renewal, reporting `N/A` where the ceiling is the only reason it cannot, + because G5's control *is* a renewal and `max_attempts: 1` would otherwise report a correct + kernel as a failure. No new error type, no new event type, no new `StateStore` method, no new + `Control` method, and no CLI change. + +### Changed + +- **An action entry may declare `max_attempts`, and a renewal over `FAILED` can now be bounded.** + This is stricter than 0.6.1 only where an operator asks for it: an action that declares no + `max_attempts` renews without bound, exactly as before, and every document that loaded at 0.6.1 + loads unchanged. There is no default ceiling, and no value of the key means "unlimited". ### Fixed diff --git a/docs/SPEC-v0.1.md b/docs/SPEC-v0.1.md index 2d56ad2..d705cf1 100644 --- a/docs/SPEC-v0.1.md +++ b/docs/SPEC-v0.1.md @@ -336,6 +336,15 @@ When a new action arrives for an `effect_key` that already has a record: `FAILED` means the executor *proved* nothing happened (§5.5). That is the only state that permits automatic retry. +**Amendment (v0.7, `SPEC-v0.7.md` §5).** The `FAILED` row is bounded where the action's policy entry declares `max_attempts` (`schema: ctrlrun.policy/v5`): at most `max_attempts` attempts execute on one effect key, the first included. + +| Existing state | New reservation | Raised | +|---|---|---| +| `FAILED` at attempt *n*, and no `max_attempts`, or *n* + 1 ≤ `max_attempts` | allowed: attempt *n* + 1, same key | none | +| `FAILED` at attempt *n*, and *n* + 1 > `max_attempts` | refused. Where the record is read before the approval gate, nothing is written. Otherwise the store assigns attempt *n* + 1, and the record is released as `FAILED` without the executor being called | `ActionDenied(reason="attempt_ceiling")` | + +The decision is taken on the attempt number the store assigned to the reservation, after the reservation and before the executor; a read of the record before the approval gate may refuse the same renewal earlier and is never the only check. The refusal appends `EFFECT_RESERVATION_REFUSED` with `data.reason = "attempt_ceiling"` and writes a `blocked` receipt. `FAILED` is still the only state that permits an automatic retry: the ceiling removes permission from that row and grants none to any other. + ### 5.5 Executor outcome mapping The wrapped function is the executor. Its result is mapped: diff --git a/docs/SPEC-v0.7.md b/docs/SPEC-v0.7.md index 1ddc0a4..7cb9450 100644 --- a/docs/SPEC-v0.7.md +++ b/docs/SPEC-v0.7.md @@ -2847,6 +2847,72 @@ prepares it, and `statement_of`'s docstring says so. ### 12.4 Item 4: the attempt ceiling +**The three equality gates became one ordering, and there is now a place to put the next one.** §5.3 named +`policy.py:912`, `931` and `1171` and said each becomes "this version or later". Writing three more membership +tuples would have made the fourth schema's item write three more again, so the comparison is one function, +`_at_least(schema, minimum)`, reading `SUPPORTED_SCHEMAS`'s order. `require_v3` and `require_v4` stay separate +functions, for `require_v4`'s own reason: the *consequences* differ per key and the sentence an operator reads +is the point. A name not in `SUPPORTED_SCHEMAS` is treated as too old, which is the fail-closed direction and is +unreachable from `Policy._from_document`, where an unknown schema is refused before any gate runs. + +**Naming the line needed the document's marks back, and they are gone by the time an entry is parsed.** +`strict_load` hands `_parse_entry` a plain mapping, and PyYAML drops a node's marks the moment it constructs one: +`construct_yaml_map` builds a bare `dict` and copies into it, so even a mapping subclass returned from +`construct_mapping` would not survive. Two designs were rejected before the one that shipped. Carrying marks +through the parse means a mapping type every caller of `strict_load` inherits, `authority.py` included, for a +message on a path that refuses the document anyway. Searching the text for `max_attempts:` finds the wrong action +in a document with two. What ships instead is `yaml.compose` **on the refusal path only**: the text is threaded +from `from_yaml` to `_from_document` to `_parse_entry` as a `line_of` callable, and the second parse happens once, +for a document that is about to be refused, and asks the loader for the one mark the message needs. Where the text +is not available, which is only `_from_document`'s own default, the message omits the line rather than inventing +one. + +**One comparison for both defences, so they cannot drift.** `Control._over_the_ceiling(ceiling, attempt)` is the +whole of "past the ceiling", and the fast path and the check both call it. Two spellings of the same comparison +would be a second definition to keep right, and §5.5's argument is that the two defences are *independent in what +they read* (a record before reserving, a reservation's assigned number) and identical in what they conclude. The +tests keep them apart by the evidence each leaves, which is what §5.5 asks for, rather than by patching one of two +comparisons. + +**What the fast path may refuse is normative and is now a single `if`.** `_ceiling_fast_path` returns `None` for +any record that is not `FAILED`. An earlier draft refused at or above the ceiling whatever the state, which reads +as stricter and is worse: it would refuse an `AMBIGUOUS` record with `attempt_ceiling` instead of letting the +reservation raise `AmbiguousEffect`, it would take T245's and G15's only route to the check away, and it would put +a `blocked` receipt saying `attempt_ceiling` on an effect whose outcome nobody knows. + +**The observe-mode half is in two places because enforce mode's order is.** The fast path's `would_have` entry is +recorded in `execute`, before `_observed` is called, because in enforce mode the fast path runs before the approval +gate and `_Observation.block` keeps the **first** reason; recording it inside `_observed` would let +`approval_required` win a race enforce mode does not have. The check's half is in `_observed`, after +`_observe_secure` returns, and reads `Policy.max_attempts` directly rather than taking `_ceiling`'s warning path: +a reservation exists there, so an effect key exists, so the warning cannot apply. + +**G15's two `N/A` reasons are decided by selecting twice, and so is G5's one.** `select()` gained +`needs_renewal`, `needs_ceiling` and `ceiling_bound`, and the precedence §8.9 states is implemented as a second +`select()` with the ceiling filter removed: G5 prints `CEILING_FORBIDS_RENEWAL` only where that second selection +finds something, and otherwise `unselected()`'s sentence, which is what keeps a deny-only or ungranted document +from being told its ceilings took the guarantee away. The same shape gives G15 `CEILING_ABOVE_BOUND` only where an +action does declare a ceiling and every one is above 100. + +**G14 is not amended here, because it does not exist on this branch.** §8.9 says item 4 makes the change for both +G5 and G14 "since item 3 lands G14 before `max_attempts` exists". Item 3 is a parallel lane and had not merged +when this was built, so `verify/scenarios.py` has no `g14` to amend. The mechanism it needs is here and is the one +G5 uses: `select(needs_effect=True, needs_renewal=True)` and `_renewal_unselected(...)` for the reason. Whichever +of items 3 and 4 rebases second wires G14 to them, in two lines. + +**The refusal's `ActionDenied` yields to the store's.** Where the record moved on between the reservation and the +release, `begin_execution` or `fail_effect` refuses, and §5.7 says that refusal propagates after the `blocked` +receipt. So `_refuse_ceiling` keeps the store's exception and raises it in place of `ActionDenied`, after +appending the event and writing the receipt: the caller is told the truer thing, which is that the key is not +theirs any more, and `v0.1 §5.5`'s rule that a store's refusal propagates is not weakened by a path that refuses +for a different reason. + +**T247 asserts a property, not an interleaving, and says so.** Six OS processes race renewals of one key on +Postgres under a ceiling of three. Racing processes do not reliably stall between a `SELECT` and an `UPDATE`, so +the test cannot claim to open item 3a's window and does not: it asserts that the total number of executor calls +never exceeds the ceiling, that at least one was made, and that at least one process was refused with +`attempt_ceiling`, so a run in which nothing contended fails rather than passing quietly. + ### 12.5 Item 5: precondition fingerprints ### 12.6 Item 6: the release diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index a148b92..de016d7 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -39,6 +39,7 @@ RECONCILED_UNKNOWN, RESOLVED_BY_RECONCILE, UNRESOLVED_EFFECT, + EffectState, ReconcileOutcome, Reservation, resolve_effect_key, @@ -75,6 +76,7 @@ BLOCKED_AMBIGUOUS, BLOCKED_APPROVAL_MISMATCH, BLOCKED_APPROVAL_REQUIRED, + BLOCKED_ATTEMPT_CEILING, BLOCKED_DUPLICATE, BLOCKED_IN_PROGRESS, Event, @@ -328,6 +330,9 @@ def __init__( #: appended once however many actions read it. self._skew_reported: ClockSkew | None = None self._skew_warned: set[str] = set() + #: SPEC-v0.7 §5.3 — the actions already warned about for a ceiling that can count + #: nothing, so the warning is one per action per Control and not one per call. + self._ceiling_warned: set[str] = set() @classmethod def from_file( @@ -724,6 +729,29 @@ def execute( reason=evaluation.reason, action_id=action.action_id, ) + # SPEC-v0.7 §5.5 — the ceiling's fast path, between policy and the approval gate. It + # saves a write, it saves a presented approval from being spent, and it saves a human + # from being asked about an attempt that could never run (`v0.3 §4.3`). It is never the + # guarantee: the check on the number the store assigned is, below. + ceiling = self._ceiling(action, effect_key) + refused_early = self._ceiling_fast_path(effect_key, ceiling) + if refused_early is not None: + if observation is not None: + # §5.7 — recorded and not enforced. Here rather than inside `_observed`, because + # enforce mode decides this before the approval gate and `_Observation.block` + # keeps the first reason: recording it later would let `approval_required` win a + # race that enforce mode does not have. + observation.block(BLOCKED_ATTEMPT_CEILING) + else: + self._refuse_ceiling( + action, + evaluation, + started_at, + effect_key, + attempt=refused_early, + ceiling=ceiling, + reserved=False, + ) if observation is not None: return self._observed( action, evaluation, executor, effect_key, started_at, observation, reconciler, held @@ -732,6 +760,21 @@ def execute( action, evaluation, started_at, effect_key, held, reconciler ) attempt = 1 if reservation is None else reservation.attempt + # SPEC-v0.7 §5.5 — the check, on the attempt number the store **assigned**, after the + # reservation and before the executor. Two callers who both read N-1 both pass the fast + # path above; only this one stops the second, because the reservation is the only write + # that assigns the number atomically. + if reservation is not None and self._over_the_ceiling(ceiling, reservation.attempt): + self._refuse_ceiling( + action, + evaluation, + started_at, + effect_key, + attempt=reservation.attempt, + ceiling=ceiling, + reserved=True, + approval=approval, + ) if effect_key is not None: try: @@ -783,6 +826,13 @@ def _observed( ) held_key = None if reservation is None else effect_key attempt = 1 if reservation is None else reservation.attempt + # SPEC-v0.7 §5.7 — the check, in observe mode: recorded and not enforced, because + # observe mode suppresses CTRLRun's decisions and not the record of an effect that + # happened. The fast path's half is recorded in `execute`, before this is reached. + if reservation is not None and self._over_the_ceiling( + self._policy.max_attempts(action.name), reservation.attempt + ): + observation.block(BLOCKED_ATTEMPT_CEILING) if held_key is not None: try: self._store.begin_execution(held_key, action.action_id) @@ -1646,6 +1696,126 @@ def _refused( effect_key=effect_key, ) + # --- the attempt ceiling (SPEC-v0.7 §5) --------------------------------------------- + + def _ceiling(self, action: Action, effect_key: str | None) -> int | None: + """This action's `max_attempts`, or `None` where the operator named none (§5.3). + + **A ceiling on an action that resolves no effect key counts nothing**, and is warned + about once per action rather than refused at load: the `effect:` template may come from + the decorator, which the policy cannot see (`v0.2 §3.2`), so the loader cannot tell a + ceiling that will count from one that cannot. Same treatment as a `reconcile` hook with + no key, and for the same reason. + """ + ceiling = self._policy.max_attempts(action.name) + if ceiling is not None and effect_key is None and action.name not in self._ceiling_warned: + self._ceiling_warned.add(action.name) + _LOG.warning( + "%s: max_attempts is %d but this call resolves no effect key, so there is no " + "record to count attempts on and nothing is bounded. Declare effect= on " + "@protect, or an 'effect:' template in the policy", + action.name, + ceiling, + ) + return ceiling + + @staticmethod + def _over_the_ceiling(ceiling: int | None, attempt: int) -> bool: + """Whether this attempt number is past the operator's ceiling (SPEC-v0.7 §5.5). + + One comparison for both defences, so there is one definition of "past the ceiling" and + not two that can drift. `None` is no ceiling, which is `v0.1 §5.4` exactly. + """ + return ceiling is not None and attempt > ceiling + + def _ceiling_fast_path(self, effect_key: str | None, ceiling: int | None) -> int | None: + """The attempt number a read before the approval gate refuses, or `None` (§5.5). + + **It refuses only a `FAILED` record**, normatively. A record in any other state, + `AMBIGUOUS` above all, passes untouched to the reservation, which refuses or reconciles + it exactly as at 0.6.1; T245's route and G15 both depend on that, and a fast path that + answered for an `AMBIGUOUS` record would take the check's only test away from it. + + It is a fast path and **never the guarantee**: two callers who both read attempt N-1 + both pass it. What it buys is that the ordinary sequential case never writes, never + spends a presented approval and never asks a human. + """ + if ceiling is None or effect_key is None: + return None + record = self._store.get_effect(effect_key) + if record is None or record.state is not EffectState.FAILED: + return None + attempt = record.attempt + 1 + return attempt if self._over_the_ceiling(ceiling, attempt) else None + + def _refuse_ceiling( + self, + action: Action, + evaluation: Evaluation, + started_at: datetime, + effect_key: str | None, + *, + attempt: int, + ceiling: int | None, + reserved: bool, + approval: Approval | None = None, + ) -> NoReturn: + """Refuse one attempt for the operator's ceiling, and say so in the history (§5.5). + + Above the ceiling the executor is not called. Where a reservation was taken, the record + is released as `FAILED` through `begin_execution` and `fail_effect`, with an error naming + the ceiling: `FAILED` is true here, because nothing ran. `EXECUTION_STARTED` is **not** + appended, because that event is the claim that something started. + + `ActionDenied` and not `DuplicateEffect`, `AmbiguousEffect` or `NotExecuted`: it is the + only type in the closed set whose meaning is true, "the action may not run, and `reason` + says why". `max_attempts` is a policy saying no, and an agent loop's `except ActionDenied` + is written for exactly that. The receipt is `blocked` rather than `denied` because it + describes what stopped the attempt, which is the effect's own history, and it keeps the + decision the policy actually reached (`v0.1 §6.1`, `§4.2 A1`). + """ + error = f"attempt {attempt} refused: max_attempts is {ceiling} (SPEC-v0.7 §5)" + released: CTRLRunError | None = None + if reserved and effect_key is not None: + try: + self._store.begin_execution(effect_key, action.action_id) + self._store.fail_effect(effect_key, action.action_id, error) + except (DuplicateEffect, AmbiguousEffect) as refused: + # §5.7 — the record moved on while the ceiling was deciding. The refusal + # propagates after the `blocked` receipt, as `v0.1 §5.5` has a store's refusal + # propagate: `AMBIGUOUS` is not something this path may collapse to `FAILED`. + released = refused + presented = approval.approval_id if approval is not None else None + self._append( + EventType.EFFECT_RESERVATION_REFUSED, + action, + { + "reason": BLOCKED_ATTEMPT_CEILING, + "attempt": attempt, + "max_attempts": ceiling, + }, + effect_key, + approval=approval, + ) + self._record( + action, + evaluation, + ReceiptResult.BLOCKED, + started_at, + error=error, + approval=approval, + approver=self._approver_of(presented), + effect_key=effect_key, + attempt=attempt, + ) + if released is not None: + raise released + raise ActionDenied( + f"{action.name} denied: {error}", + reason=BLOCKED_ATTEMPT_CEILING, + action_id=action.action_id, + ) + def _unrecorded( self, action: Action, diff --git a/src/ctrlrun/policy.py b/src/ctrlrun/policy.py index 1b8874b..32d7f36 100644 --- a/src/ctrlrun/policy.py +++ b/src/ctrlrun/policy.py @@ -19,7 +19,7 @@ from dataclasses import dataclass, field from datetime import date, datetime, time from enum import StrEnum -from functools import cached_property +from functools import cached_property, partial from pathlib import Path from types import MappingProxyType from typing import Any, Final, Literal @@ -49,14 +49,37 @@ #: those three keys need `v4`. POLICY_SCHEMA_V4: Final = "ctrlrun.policy/v4" -#: All of them, newest last, for the message an unknown schema produces. +#: SPEC-v0.7 §5.3 — required by any document using `max_attempts:`. An 0.6.1 reader refuses a +#: `v5` document outright, which is the fail-closed direction: a reader that ignored the key +#: would renew without a ceiling, which is the behaviour the key exists to bound. +POLICY_SCHEMA_V5: Final = "ctrlrun.policy/v5" + +#: All of them, newest last, for the message an unknown schema produces. **In version order**, +#: which `_at_least` reads: a version added out of order would make every gate below lie. SUPPORTED_SCHEMAS: Final = ( POLICY_SCHEMA, POLICY_SCHEMA_V2, POLICY_SCHEMA_V3, POLICY_SCHEMA_V4, + POLICY_SCHEMA_V5, ) + +def _at_least(schema: str, minimum: str) -> bool: + """Whether `schema` is `minimum` or a later version (SPEC-v0.7 §5.3). + + Each version is a **superset** of the one before: a `v5` document may use every key any + earlier version allows. Three gates in this module compared for equality instead, which was + right while `v4` was the newest and became wrong the moment it was not; `require_v3`'s own + comment predicted it. An unknown schema is refused before any of them runs, so a name that + is not in `SUPPORTED_SCHEMAS` cannot reach here from `Policy._from_document`; where one does, + from `authority.py`'s standalone path, it is treated as too old, which is fail-closed. + """ + if schema not in SUPPORTED_SCHEMAS or minimum not in SUPPORTED_SCHEMAS: + return False + return SUPPORTED_SCHEMAS.index(schema) >= SUPPORTED_SCHEMAS.index(minimum) + + #: SPEC-v0.3 §6.1 — the two values of the top-level `mode:` key, and nothing else. Absent #: means `enforce`: the fail-closed default, so a document that predates the key enforces. MODE_KEY: Final = "mode" @@ -123,12 +146,25 @@ ), } +#: SPEC-v0.7 §5.3 — the action-entry key that needs `ctrlrun.policy/v5`, and the same sentence: +#: what an older reader would do with the document if it ignored the key. +_V5_ENTRY_KEYS: Final[Mapping[str, str]] = { + "max_attempts": ( + "an older reader would ignore the ceiling and renew over `FAILED` without bound, which " + "is the behaviour this key exists to stop" + ), +} + _RULE_KEYS: Final = frozenset({"when", "decision", "controls"}) #: SPEC-v0.2 §3.1 — the keys `ctrlrun.policy/v2` adds to an action entry. The gateway has no #: decorator to carry an effect template, so the policy file has to. _V2_ENTRY_KEYS: Final = frozenset({"effect", "resource", "mcp"}) -_ENTRY_KEYS: Final = frozenset({"decision", "rules", "controls", "data"}) | _V2_ENTRY_KEYS +_ENTRY_KEYS: Final = ( + frozenset({"decision", "rules", "controls", "data"}) + | _V2_ENTRY_KEYS + | frozenset(_V5_ENTRY_KEYS) +) #: And the closed key set of the `mcp` mapping, which is one key wide. _MCP_KEYS: Final = frozenset({"not_executed_on_error"}) @@ -483,6 +519,9 @@ class _ActionPolicy: controls: tuple[str, ...] = () #: §7.4 — which of this action's arguments carry which class of data. data: Mapping[str, DataLabel] = field(default_factory=dict) + #: SPEC-v0.7 §5.3 — how many attempts may execute on one effect key, the first included, or + #: `None` where the entry names no ceiling. `None` is today's behaviour and is not a number. + max_attempts: int | None = None def data_scope(self, arguments: Mapping[str, Any]) -> frozenset[str]: """The labels present in **the arguments actually supplied** (SPEC-v0.6 §7.4). @@ -661,10 +700,10 @@ def from_file(cls, path: str | os.PathLike[str] | None = None) -> Policy: @classmethod def from_yaml(cls, text: str, *, source: str = "") -> Policy: """Parse and validate a policy document. Anything malformed raises `PolicyError`.""" - return cls._from_document(strict_load(text, source), source) + return cls._from_document(strict_load(text, source), source, text) @classmethod - def _from_document(cls, document: object, source: str) -> Policy: + def _from_document(cls, document: object, source: str, text: str | None = None) -> Policy: if not isinstance(document, Mapping): raise PolicyError( f"{source}: policy must be a mapping with 'schema' and 'actions' keys, " @@ -708,7 +747,13 @@ def _from_document(cls, document: object, source: str) -> Policy: if not isinstance(name, str) or not name: raise PolicyError(f"{source}: action names must be non-empty strings, got {name!r}") actions[name] = _parse_entry( - entry, f"{source}: action {name!r}", str(schema), frozenset(controls) + entry, + f"{source}: action {name!r}", + str(schema), + frozenset(controls), + # SPEC-v0.7 §5.3 — the line a refused key sits on, recovered from the document's + # own marks and only where a refusal is about to name one. + line_of=partial(_entry_key_line, text, name), ) return cls( actions=MappingProxyType(actions), @@ -745,6 +790,17 @@ def mcp_options(self, action_name: str) -> McpOptions: entry = self.actions.get(action_name) return _DEFAULT_MCP_OPTIONS if entry is None else entry.mcp + def max_attempts(self, action_name: str) -> int | None: + """This action's attempt ceiling, or `None` (SPEC-v0.7 §5.3). + + `None` means the operator named no ceiling, which is 0.6.1's behaviour exactly: a renewal + over `FAILED` is admitted without bound (`v0.1 §5.4`). It is not a number and never + defaults to one, because any default would refuse at 0.7.0 a renewal that succeeded at + 0.6.1, and would be a bound nobody chose (§5.4). + """ + entry = self.actions.get(action_name) + return None if entry is None else entry.max_attempts + def evaluate(self, action: Action) -> Evaluation: """Decide an action. No side effects; an unlisted action is denied (SPEC-v0.1 §3.4).""" entry = self.actions.get(action.name) @@ -932,8 +988,9 @@ def require_v3(document: Mapping[Any, Any], schema: str, source: str) -> None: nothing else, so the check has to exist on both paths rather than on whichever runs first. """ # v4 is a superset: a `v4` document may use every `v3` key. Comparing for equality here was - # right while v3 was the newest and becomes a bug the moment it is not. - if schema in (POLICY_SCHEMA_V3, POLICY_SCHEMA_V4): + # right while v3 was the newest and becomes a bug the moment it is not. SPEC-v0.7 §5.3: the + # membership test that replaced it had the same shape, so `v5` reads it through `_at_least`. + if _at_least(schema, POLICY_SCHEMA_V3): return for key, consequence in _V3_TOP_LEVEL_KEYS.items(): if key in document: @@ -952,7 +1009,7 @@ def require_v4(document: Mapping[Any, Any], schema: str, source: str) -> None: check at all" and "an older reader would refuse the document outright" call for different reactions. """ - if schema == POLICY_SCHEMA_V4: + if _at_least(schema, POLICY_SCHEMA_V4): return for key, consequence in _V4_TOP_LEVEL_KEYS.items(): if key in document: @@ -1172,7 +1229,12 @@ def _parse_cited(value: object, where: str, known: frozenset[str]) -> tuple[str, def _parse_entry( - entry: object, where: str, schema: str, known: frozenset[str] = frozenset() + entry: object, + where: str, + schema: str, + known: frozenset[str] = frozenset(), + *, + line_of: Callable[[str], int | None] = lambda key: None, ) -> _ActionPolicy: if not isinstance(entry, Mapping): raise PolicyError( @@ -1192,12 +1254,19 @@ def _parse_entry( cited = _parse_cited(entry.get("controls"), where, known) for key, consequence in _V4_ENTRY_KEYS.items(): - if key in entry and schema != POLICY_SCHEMA_V4: + if key in entry and not _at_least(schema, POLICY_SCHEMA_V4): raise PolicyError( f"{where}: {key!r} needs 'schema: {POLICY_SCHEMA_V4}'; this document declares " f"{schema!r}, and {consequence}" ) + for key, consequence in _V5_ENTRY_KEYS.items(): + if key in entry and not _at_least(schema, POLICY_SCHEMA_V5): + raise PolicyError( + f"{where}: {key!r}{_at_line(line_of(key))} needs 'schema: {POLICY_SCHEMA_V5}'; " + f"this document declares {schema!r}, and {consequence}" + ) labels = _parse_data(entry.get("data"), where) + ceiling = _parse_max_attempts(entry, where, line_of) if has_decision: return _ActionPolicy( @@ -1208,6 +1277,7 @@ def _parse_entry( mcp=mcp, controls=cited, data=MappingProxyType(labels), + max_attempts=ceiling, ) rules = entry["rules"] @@ -1223,9 +1293,86 @@ def _parse_entry( mcp=mcp, controls=cited, data=MappingProxyType(labels), + max_attempts=ceiling, ) +def _at_line(line: int | None) -> str: + """` on line N`, or nothing where the document's marks could not be recovered.""" + return "" if line is None else f" on line {line}" + + +def _parse_max_attempts( + entry: Mapping[Any, Any], where: str, line_of: Callable[[str], int | None] +) -> int | None: + """The attempt ceiling, validated at load (SPEC-v0.7 §5.3). + + **At load, and naming the key, the action and the line**, so a malformed ceiling fails the + policy rather than the execution: a document that cannot say how many attempts it permits is + a document nobody should deploy, and finding out at the fourth dispatch is finding out late. + + `bool` is refused although Python makes it an `int` (`v0.1 §3.2`): `max_attempts: true` is a + typo for a number and not a ceiling of one. `0` is refused rather than read as "unlimited" + (`v0.7 §1.1`: no value of this key relaxes it) or as "never run" (`max_attempts` counts what + executes, and an action that may never execute is a `decision: deny`). There is no upper + bound: a very large ceiling is the operator's statement that they meant it. + """ + if "max_attempts" not in entry: + return None + value = entry["max_attempts"] + at = _at_line(line_of("max_attempts")) + if isinstance(value, bool) or not isinstance(value, int): + raise PolicyError( + f"{where}: 'max_attempts'{at} must be an integer of at least 1, got " + f"{_type_name(value)} {value!r}. It counts the attempts that may execute on one " + "effect key, the first included; remove the key for no ceiling" + ) + if value < 1: + raise PolicyError( + f"{where}: 'max_attempts'{at} must be an integer of at least 1, got {value!r}. " + "It counts the attempts that may execute on one effect key, the first included, so " + "there is no ceiling below 1; remove the key for no ceiling" + ) + return value + + +def _entry_key_line(text: str | None, action: str, key: str) -> int | None: + """The 1-based line `actions: : :` sits on, or `None`. + + Composed on demand, on the refusal path only, rather than carried through the parse: the + loader hands `_parse_entry` a plain document, and PyYAML drops a node's marks the moment it + constructs one. Composing again reads the same text with the same loader and asks it for the + one mark the message needs, at the cost of a second parse of a document that is about to be + refused. `_StrictLoader.construct_mapping` never runs here, so the duplicate-key refusal is + unaffected either way: that one already fired, in `strict_load`, before this could. + """ + if text is None: + return None + try: + root = yaml.compose(text, Loader=_StrictLoader) + except (yaml.YAMLError, ValueError, OverflowError): # pragma: no cover - strict_load ran first + return None + node = _child(_child(root, "actions"), action) + found = None if node is None else _key_node(node, key) + return None if found is None else int(found.start_mark.line) + 1 + + +def _child(node: Any, key: str) -> Any: # noqa: ANN401 - PyYAML ships no stubs + found = _key_node(node, key) + if found is None: + return None + return next(value for name, value in node.value if name is found) + + +def _key_node(node: Any, key: str) -> Any: # noqa: ANN401 - PyYAML ships no stubs + if not isinstance(node, yaml.MappingNode): + return None + for name, _ in node.value: + if isinstance(name, yaml.ScalarNode) and name.value == key: + return name + return None + + def _reject_v2_keys_under_v1(entry: Mapping[Any, Any], where: str, schema: str) -> None: """A v2 key in a v1 document is a load error naming the key and the schema (§3.1). diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index e1e8380..c022445 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -56,7 +56,13 @@ BLOCKED_IN_PROGRESS: Final = "in_progress" BLOCKED_AMBIGUOUS: Final = "ambiguous" -#: The four that mean "the effect state or a presented approval would have stopped it", as +#: SPEC-v0.7 §5.5 — the attempt ceiling's refusal, in the same three places: `ActionDenied.reason`, +#: `EFFECT_RESERVATION_REFUSED.data.reason`, and `would_have.blocked_reason` in observe mode. It is +#: a value of existing fields and not a new type: an operator's `max_attempts` is a policy saying +#: no, and an agent loop's `except ActionDenied` is written for exactly that. +BLOCKED_ATTEMPT_CEILING: Final = "attempt_ceiling" + +#: The five that mean "the effect state or a presented approval would have stopped it", as #: opposed to a decision that would have. `ctrlrun stats` counts them as one line (§6.4). BLOCKED_BY_STATE: Final = frozenset( { @@ -64,6 +70,7 @@ BLOCKED_DUPLICATE, BLOCKED_IN_PROGRESS, BLOCKED_AMBIGUOUS, + BLOCKED_ATTEMPT_CEILING, } ) diff --git a/src/ctrlrun/verify/guarantees.py b/src/ctrlrun/verify/guarantees.py index 7e495d4..6702b8e 100644 --- a/src/ctrlrun/verify/guarantees.py +++ b/src/ctrlrun/verify/guarantees.py @@ -62,6 +62,18 @@ class Guarantee: "v0.7 §8 T213", ), ), + Guarantee( + "G15", + "a renewal past the operator's ceiling is refused", + ( + "v0.1 §5.4", + "v0.7 §8 T240", + "v0.7 §8 T241", + "v0.7 §8 T242", + "v0.7 §8 T245", + "v0.7 §8 T245b", + ), + ), ) #: By id, for `--only` and for the report. Insertion order is catalogue order. @@ -141,6 +153,27 @@ class Guarantee: "to diverge from; pass --store-url postgresql://… to grade this" ) +#: SPEC-v0.7 §8.9 — G15's two N/A reasons, and the bound behind the second. Every loop verify +#: runs is bounded (`v0.4 §3.6`), so a ceiling above what verify will drive is a statement about +#: the document *and* about verify's stated bound, on the precedent of `GRANT_ALREADY_EXPIRED`. +NO_CEILING_DECLARED: Final = ( + "no action verify can drive to allow or approve declares both `effect:` and `max_attempts`" +) +CEILING_BOUND: Final = 100 +CEILING_ABOVE_BOUND: Final = ( + f"every declared max_attempts is above verify's bound of {CEILING_BOUND} attempts" +) + +#: SPEC-v0.7 §8.9 — the reason G5 (and G14, when item 3 lands it) reports where the operator's +#: ceiling is the *only* thing that makes a renewal unselectable. Printed only where selecting +#: again without the ceiling filter does find something; otherwise `unselected()`'s reason wins, +#: because a document whose uncapped action is deny-only or ungranted is not a document whose +#: ceilings took the guarantee away. +CEILING_FORBIDS_RENEWAL: Final = ( + "every action with an `effect:` template that verify can select (a decision of allow or " + "approve under a grant that covers it) declares max_attempts: 1, so no renewal can happen" +) + #: `--only` (§4.6). NOT_SELECTED: Final = "not selected" @@ -151,6 +184,9 @@ class Guarantee: "BY_ID", "CANDIDATE_BOUND", "CATALOGUE", + "CEILING_ABOVE_BOUND", + "CEILING_BOUND", + "CEILING_FORBIDS_RENEWAL", "CONTROL_FAILED", "EFFECT_TEMPLATE_NOTE", "EVERY_ACTION_DENIED", @@ -161,6 +197,7 @@ class Guarantee: "NO_ACTIONS", "NO_APPROVE_RULE", "NO_AUTHORITY_SECTION", + "NO_CEILING_DECLARED", "NO_DELEGABLE_GRANT", "NO_EFFECT_TEMPLATE", "NO_EXPIRES_AT", diff --git a/src/ctrlrun/verify/scenarios.py b/src/ctrlrun/verify/scenarios.py index 344aad7..2e406a4 100644 --- a/src/ctrlrun/verify/scenarios.py +++ b/src/ctrlrun/verify/scenarios.py @@ -67,6 +67,7 @@ ) from ..policy import Condition, Decision, Policy, _ActionPolicy, _Rule, discover_policy_path from ..receipt import ( + BLOCKED_ATTEMPT_CEILING, Event, EventType, Receipt, @@ -690,6 +691,9 @@ def select( *, decisions: Sequence[Decision] = (Decision.ALLOW, Decision.APPROVE), needs_effect: bool = False, + needs_renewal: bool = False, + needs_ceiling: bool = False, + ceiling_bound: int | None = None, grant_filter: Callable[[Grant], bool] | None = None, mutation: Mapping[str, Any] | None = None, ) -> _Selection | None: @@ -699,11 +703,24 @@ def select( covers the action (§3.4), because an action nothing authorizes is refused by the authority axis before the policy axis is ever reached (`v0.3 §4.3`), and a scenario built on one would exercise a different guarantee than the one it claims. + + SPEC-v0.7 §8.9 adds the ceiling axis, and it cuts both ways. `needs_renewal` skips an + action whose `max_attempts` forbids one, because G5's control *is* a renewal and under + `max_attempts: 1` a correct kernel refuses it, which would be reported as a `fail`. + `needs_ceiling` and `ceiling_bound` are G15's own requirement, which is the opposite: it + needs an action that declares one, and one verify can drive to the top of. """ self._grant_miss = None for name in sorted(self.policy.actions): if needs_effect and self.policy.effect_template(name) is None: continue + ceiling = self.policy.max_attempts(name) + if needs_renewal and ceiling is not None and ceiling < 2: + continue + if needs_ceiling and ceiling is None: + continue + if ceiling_bound is not None and ceiling is not None and ceiling > ceiling_bound: + continue for decision in decisions: synthesized = self._synthesize(name, decision, mutation) if synthesized is None: @@ -1353,16 +1370,33 @@ def _contend( pids.add(int(document["pid"])) return read, len(list(counters.iterdir())), pids + def _renewal_unselected(self, reason: str) -> tuple[str, dict[str, Any]]: + """Why a guarantee that needs a renewal found nothing (SPEC-v0.7 §8.9). + + **Precedence, and it is the whole of this function.** The ceiling sentence is printed + only where the ceiling is the *only* reason nothing is selectable, which is established + by selecting again with the ceiling filter removed: where that selection also finds + nothing, the reason is `unselected()`'s, as before. A document whose uncapped action is + deny-only, or allowed and covered by no grant, has not had its guarantee taken away by a + ceiling, and saying so would be a false `N/A` reason on an `N/A` that is otherwise right. + """ + without_the_ceiling = self.select(needs_effect=True) + if without_the_ceiling is not None: + return reg.CEILING_FORBIDS_RENEWAL, {} + # `select()` has just run again, so `_grant_miss` describes the unfiltered attempt, + # which is the one `unselected()` is being asked about. + return self.unselected(reason), self.unselected_detail(reg.EFFECT_TEMPLATE_NOTE) + # --- G5: an ambiguous outcome blocks a blind retry ----------------------------------- def g5(self) -> GuaranteeResult: - selection = self.select(needs_effect=True) + # SPEC-v0.7 §8.9 — only an action whose ceiling admits a renewal, because G5's control + # is a renewal: under `max_attempts: 1` a correct kernel refuses it, and grading the + # document anyway would report that correct kernel as `fail`. + selection = self.select(needs_effect=True, needs_renewal=True) if selection is None: - return self.na( - "G5", - self.unselected(reg.NO_EFFECT_TEMPLATE), - **self.unselected_detail(reg.EFFECT_TEMPLATE_NOTE), - ) + reason, detail = self._renewal_unselected(reg.NO_EFFECT_TEMPLATE) + return self.na("G5", reason, **detail) control, store, recorder, _ = self._control_for("G5", selection) def body(detail: dict[str, Any]) -> None: @@ -2301,6 +2335,170 @@ def body(detail: dict[str, Any]) -> None: for store in opened: store.close() + # --- G15: a renewal past the operator's ceiling is refused ---------------------------- + + def g15(self) -> GuaranteeResult: + """SPEC-v0.7 §8.9. Graded where the document declares a ceiling verify can reach. + + **The route is the guarantee.** An earlier draft drove N+1 sequential `NotExecuted` + attempts, which §5.5's fast path alone refuses, so G15 passed with the check on the + assigned attempt number deleted: green with the guarantee's own mechanism gone. This + drives §5.5's public route instead. Attempt N ends `AMBIGUOUS` through a `TimeoutError`, + so the fast path reads a record that is not `FAILED` and lets the call through; attempt + N+1's `reconcile` hook answers `not_executed`, the record moves to `FAILED` at N, the + second take renews it to N+1, and only the check after the reservation can refuse that. + + **The control is that every attempt up to N executed**, so for N of 2 or more a kernel + that refused every renewal fails here. At N = 1 there is no renewal to admit and the + control cannot tell such a kernel from a correct one; §8.9 says what that leaves + ungraded, and the G5 amendment is the other half of the same sentence. + """ + selection = self.select( + needs_effect=True, needs_ceiling=True, ceiling_bound=reg.CEILING_BOUND + ) + if selection is None: + if self.select(needs_effect=True, needs_ceiling=True) is not None: + return self.na("G15", reg.CEILING_ABOVE_BOUND) + return self.na("G15", self.unselected(reg.NO_CEILING_DECLARED)) + ceiling = self.policy.max_attempts(selection.action) + assert ceiling is not None + control, store, recorder, _ = self._control_for("G15", selection) + + def body(detail: dict[str, Any]) -> None: + key = str(selection.effect_key) + detail["max_attempts"] = ceiling + remote = _Executor(_raises(NotExecuted("ctrlrun-verify: the remote did nothing"))) + for _ in range(ceiling - 1): + proposal = selection.build() + # `ActionDenied` too, and deliberately: a kernel that refused a renewal *below* + # the ceiling is what the control below is for, and letting it escape here would + # report that kernel as verify's own internal error rather than as a failure. + with suppress(NotExecuted, ActionDenied): + self.execute( + control, + proposal, + remote, + key, + self.approve(control, store, proposal, selection), + ) + _expect_control( + remote.calls == ceiling - 1, + f"the {ceiling - 1} renewals below the ceiling are admitted", + f"the executor was called {remote.calls} times", + ) + + lost = _Executor( + _raises( + TimeoutError("ctrlrun-verify: the response was lost after the remote acted") + ) + ) + timed_out = selection.build() + with suppress(TimeoutError, ActionDenied): + self.execute( + control, + timed_out, + lost, + key, + self.approve(control, store, timed_out, selection), + ) + record = store.get_effect(key) + _expect_control( + record is not None + and record.state is EffectState.AMBIGUOUS + and record.attempt == ceiling, + f"attempt {ceiling} leaves the record AMBIGUOUS at {ceiling}", + f"the record is {None if record is None else (record.state, record.attempt)}", + ) + _expect_control( + lost.calls == 1, + f"attempt {ceiling} reached the executor", + f"the executor was called {lost.calls} times", + ) + + over = selection.build() + before = len(recorder.events) + refusal = self.refused( + lambda: self._reconciled_attempt(control, over, key, selection, store), + (ActionDenied,), + f"ActionDenied on attempt {ceiling + 1}", + f"the attempt past max_attempts: {ceiling} reached the remote", + ) + _expect( + getattr(refusal, "reason", None) == BLOCKED_ATTEMPT_CEILING, + "the refusal names the ceiling", + f"it was refused with reason {getattr(refusal, 'reason', None)!r}", + ) + _expect( + remote.calls + lost.calls == ceiling, + f"the executor was called exactly {ceiling} times", + f"it was called {remote.calls + lost.calls} times", + ) + appended = [event for event in recorder.events[before:] if event.effect_key == key] + types = [str(event.type) for event in appended] + _expect( + "EFFECT_RESERVED" in types + and "EFFECT_RESERVATION_REFUSED" in types + and types.index("EFFECT_RESERVED") < types.index("EFFECT_RESERVATION_REFUSED"), + "the store reserved the attempt and the check then refused it", + f"the refused attempt appended {types}", + ) + refused_event = next( + event for event in appended if str(event.type) == "EFFECT_RESERVATION_REFUSED" + ) + _expect( + refused_event.data.get("reason") == BLOCKED_ATTEMPT_CEILING, + "EFFECT_RESERVATION_REFUSED names the ceiling", + f"its reason is {refused_event.data.get('reason')!r}", + ) + after = store.get_effect(key) + _expect( + after is not None + and after.state is EffectState.FAILED + and after.attempt == ceiling + 1, + f"the refused attempt leaves the record FAILED at {ceiling + 1}", + f"the record is {None if after is None else (after.state, after.attempt)}", + ) + blocked = _last_receipt(store, over.action_id) + _expect( + blocked is not None and blocked.result is ReceiptResult.BLOCKED, + "the refused attempt's receipt is `blocked`", + f"the receipt is {None if blocked is None else blocked.result}", + ) + + try: + return self.graded("G15", selection, store, recorder, body) + finally: + store.close() + + def _reconciled_attempt( + self, + control: Control, + action: Action, + effect_key: str, + selection: _Selection, + store: StateStore, + ) -> Receipt: + """One attempt carrying §5.5's `reconcile` hook, and an approval where one is needed. + + The hook is what makes G15's route public: it moves the `AMBIGUOUS` record the previous + attempt left to `FAILED`, so the second take renews rather than refusing, and the check + on the assigned attempt number is the only thing that can stop what follows. + """ + from ..control import with_approval + + approval_id = self.approve(control, store, action, selection) + executor = _Executor(_raises(NotExecuted("ctrlrun-verify: the remote did nothing"))) + + def attempt() -> Receipt: + return control.execute( + action, executor, effect_key, reconcile=lambda _key: "not_executed" + ) + + if approval_id is None: + return attempt() + with with_approval(approval_id): + return attempt() + @dataclass(frozen=True) class _AlteredChain: diff --git a/tests/test_attempt_cap.py b/tests/test_attempt_cap.py new file mode 100644 index 0000000..7315ebb --- /dev/null +++ b/tests/test_attempt_cap.py @@ -0,0 +1,1340 @@ +"""The attempt ceiling, `max_attempts`. Build-list item 4; SPEC-v0.7 §5, §8.4 (T240 to T252). + +`v0.1 §5.4`'s `FAILED` row permits a renewal and bounds nothing. An executor that raises +`NotExecuted` on every call is therefore an unlimited number of provider dispatches, each one +individually correct and the sequence as a whole unbounded. §5's amendment bounds it where the +operator's policy entry says so, and leaves it exactly as it was where the entry says nothing. + +**Two defences, two tests, because defence in depth hides mutations** (`CONTRIBUTING.md`). The +guarantee is the comparison against the attempt number **the store assigned**, taken after the +reservation and before the executor: two callers who both read `N-1` both pass a read taken +before reserving, and only the assigned number is atomic by construction. The read before the +approval gate is a fast path that saves a write, a spent approval and a human's attention, and +it is never the guarantee. T245 reaches the check with the fast path live, through the public +route §5.5 describes, and T245b reaches the fast path by the evidence only it leaves. + +**Every refusal is asserted by its `reason`**, never by its type alone (mutation pattern 1): an +`ActionDenied` is also what a policy denial raises, and an `EFFECT_RESERVATION_REFUSED` is also +what a duplicate or an ambiguous record appends. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import textwrap +import uuid +from contextlib import contextmanager +from datetime import timedelta +from pathlib import Path + +import pytest + +from ctrlrun import ( + ActionDenied, + AmbiguousEffect, + ApprovalRequired, + Control, + InMemoryStateStore, + NotExecuted, + Policy, + PolicyError, + SQLiteStateStore, + Suspended, + context, + protect, + with_approval, +) +from ctrlrun.effect import EffectState +from ctrlrun.receipt import BLOCKED_ATTEMPT_CEILING, EventType, ReceiptResult + +#: The reason string §5.5 freezes, and §9.2 lists. Imported rather than spelled here so a +#: change to the constant is a red test and not a silent rename of the thing an operator greps. +CEILING = BLOCKED_ATTEMPT_CEILING + +ALLOW_CEILING_3 = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 3 + decision: allow +""" + +ALLOW_NO_CEILING = """ +schema: ctrlrun.policy/v2 +actions: + stripe.refund: + effect: "refund:{payment_id}" + decision: allow +""" + +ALLOW_CEILING_1 = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 1 + decision: allow +""" + +APPROVE_CEILING_1 = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 1 + decision: approve +""" + +APPROVE_CEILING_2 = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 2 + decision: approve +""" + +OBSERVE_CEILING_1 = """ +schema: ctrlrun.policy/v5 +mode: observe +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 1 + decision: allow +""" + +ALLOW_CEILING_2 = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 2 + decision: allow +""" + +OBSERVE_CEILING_2 = """ +schema: ctrlrun.policy/v5 +mode: observe +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 2 + decision: allow +""" + +KEY = "refund:txn_1" + + +class Remote: + """An executor that counts its calls and does whatever the script says (§8.4 T241).""" + + def __init__(self, *script: object) -> None: + self.calls = 0 + self._script = list(script) + + def __call__(self) -> str: + self.calls += 1 + step = self._script[min(self.calls - 1, len(self._script) - 1)] + if isinstance(step, BaseException): + raise step + return f"re_txn_1-{self.calls}" + + +def _not_executed() -> NotExecuted: + return NotExecuted("the remote rejected it before doing anything") + + +@pytest.fixture(params=["in-memory", "sqlite"]) +def stores(request, tmp_path, fake_clock): + """A factory for stores on whichever backend, so one test can build two (T247). + + `conftest.state_store` hands out one store, and the positive control needs a second: the + same sequence under a document with no ceiling, compared record for record. + """ + made: list[object] = [] + + def make(): + store = ( + InMemoryStateStore(clock=fake_clock) + if request.param == "in-memory" + else SQLiteStateStore(tmp_path / f"state-{len(made)}.db", clock=fake_clock) + ) + made.append(store) + return store + + yield make + for store in made: + store.close() + + +def _control(document, store, fake_clock): + return Control(Policy.from_yaml(document), store, clock=fake_clock) + + +def _refund(control, remote, **kwargs): + @protect("stripe.refund", effect="refund:{payment_id}", control=control, **kwargs) + def refund(payment_id: str, amount: int) -> str: + return remote() + + return refund + + +def _call(control, remote, *, payment_id="txn_1", approval=None, **kwargs): + """One attempt, returning what it raised or the receipt it produced.""" + refund = _refund(control, remote, **kwargs) + with context(agent="refund-agent"): + if approval is None: + return refund(payment_id=payment_id, amount=200) + with with_approval(approval): + return refund(payment_id=payment_id, amount=200) + + +def _events(store, type_=None): + return [event for event in store.events() if type_ is None or event.type is type_] + + +def _refusals(store): + return _events(store, EventType.EFFECT_RESERVATION_REFUSED) + + +def _receipts(store): + return list(store.receipts()) + + +def _grant(control, store, payment_id="txn_1"): + """Verify's own approval, through the call `ctrlrun approve` makes.""" + from ctrlrun.action import Action + from ctrlrun.identity import Principal + + action = Action( + name="stripe.refund", + arguments={"payment_id": payment_id, "amount": 200}, + principal=Principal(agent="refund-agent"), + environment=control.environment, + ) + request = control.approvals.request(action, timedelta(hours=1)) + store.grant_approval(request.request_id, "ops@example.com") + return request.request_id + + +@contextmanager +def _window(store, method, interleave, *, after=True): + """Open one window inside a `Control.execute`, on purpose, and only once. + + `interleave` runs immediately after (or before) the store's `method` returns, so what the + caller does next happens in a world that has moved. That is what makes these tests + reproductions rather than approximations (mutation pattern 4): two `Control`s calling in + turn open nothing, because the second reads what the first left behind. + + Armed once, and re-entrant by construction: `interleave` runs its own `Control.execute`, + whose own call to the same method must not fire the window again. + """ + original = getattr(store, method) + state = {"armed": True, "inside": False} + + def wrapped(*args, **kwargs): + if not after: + _fire(state, interleave) + answer = original(*args, **kwargs) + if after: + _fire(state, interleave) + return answer + + setattr(store, method, wrapped) + try: + yield state + finally: + setattr(store, method, original) + + +def _fire(state, interleave): + if not state["armed"] or state["inside"]: + return + state["armed"] = False + state["inside"] = True + try: + interleave() + finally: + state["inside"] = False + + +def _stale_read(store, interleave): + """The window between the fast path's read and the reservation (§5.5). + + The fast path's `get_effect` returns what the record was, and `interleave` then moves it on + before the caller reaches `reserve_effect`: two callers who both read attempt N-1 both pass + a read taken before reserving, and only the check on the number the store **assigned** can + stop the second. + """ + return _window(store, "get_effect", interleave) + + +def _renew_and_fail(control, remote, payment_id="txn_1"): + """One whole attempt by somebody else: renew the key, dispatch, and fail.""" + with pytest.raises(NotExecuted): + _call(control, remote, payment_id=payment_id) + + +def _drive_to_failed(control, store, ceiling): + """Run `ceiling` attempts that each raise `NotExecuted`, leaving the record FAILED at N.""" + remote = Remote(*[_not_executed() for _ in range(ceiling)]) + for _ in range(ceiling): + with pytest.raises(NotExecuted): + _call(control, remote) + record = store.get_effect(KEY) + assert record is not None and record.state is EffectState.FAILED + assert record.attempt == ceiling + return remote + + +# --- T240: exactly N dispatches, then a named refusal ----------------------------------- + + +def test_T240_exactly_N_dispatches_then_a_refusal_naming_the_ceiling(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = _drive_to_failed(control, store, 3) + + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + + # The reason, not only the type: a policy denial is an `ActionDenied` too. + assert refused.value.reason == CEILING + assert remote.calls == 3 + + blocked = _receipts(store)[-1] + assert blocked.result is ReceiptResult.BLOCKED + assert "max_attempts is 3" in (blocked.error or "") + + refusal = _refusals(store)[-1] + assert refusal.data["reason"] == CEILING + assert refusal.data["attempt"] == 4 + assert refusal.data["max_attempts"] == 3 + + record = store.get_effect(KEY) + assert record.state is EffectState.FAILED + + +# --- T241: a refused attempt never calls the executor ----------------------------------- + + +def test_T241_the_fast_path_never_calls_the_executor(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = _drive_to_failed(control, store, 3) + with pytest.raises(ActionDenied): + _call(control, remote) + assert remote.calls == 3 + + +def test_T241_the_check_after_the_reservation_never_calls_the_executor(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = _reach_the_check(control, store) + assert remote.calls == 3 + + +# --- T242: the positive control -- under the ceiling, 0.6.1's behaviour ----------------- + + +def _projection(store): + """What a run left behind, in the terms 0.6.1 and 0.7 must agree on.""" + events = [(str(event.type), dict(event.data), event.effect_key) for event in store.events()] + receipts = [ + (receipt.result, receipt.attempt, receipt.effect_key) for receipt in _receipts(store) + ] + record = store.get_effect(KEY) + return events, receipts, (record.state, record.attempt) + + +def test_T242_under_the_ceiling_nothing_changes(stores, fake_clock): + capped_store = stores() + capped = _control(ALLOW_CEILING_3, capped_store, fake_clock) + _drive_to_failed(capped, capped_store, 3) + + free_store = stores() + free = _control(ALLOW_NO_CEILING, free_store, fake_clock) + _drive_to_failed(free, free_store, 3) + + assert _projection(capped_store) == _projection(free_store) + + +# --- T243: no ceiling, no change -------------------------------------------------------- + + +def test_T243_a_document_with_no_ceiling_renews_without_bound(stores, fake_clock): + store = stores() + control = _control(ALLOW_NO_CEILING, store, fake_clock) + # More attempts than any ceiling in this suite, so a hardcoded default would show up here. + remote = Remote(*[_not_executed() for _ in range(8)]) + for _ in range(8): + with pytest.raises(NotExecuted): + _call(control, remote) + assert remote.calls == 8 + assert store.get_effect(KEY).attempt == 8 + assert not [event for event in _refusals(store) if event.data.get("reason") == CEILING] + + +def test_T243_an_action_with_no_ceiling_beside_one_with_a_ceiling(stores, fake_clock): + """The key is per action: a ceiling on one action bounds nothing on another.""" + document = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 1 + decision: allow + stripe.payout: + effect: "payout:{payment_id}" + decision: allow +""" + store = stores() + control = _control(document, store, fake_clock) + remote = Remote(*[_not_executed() for _ in range(5)]) + + @protect("stripe.payout", effect="payout:{payment_id}", control=control) + def payout(payment_id: str, amount: int) -> str: + return remote() + + for _ in range(5): + with context(agent="refund-agent"), pytest.raises(NotExecuted): + payout(payment_id="txn_1", amount=200) + assert remote.calls == 5 + + +# --- T244: the loader refuses a malformed ceiling --------------------------------------- + + +def _document_with(value: str) -> tuple[str, int]: + """A v5 document whose `max_attempts` is `value`, and the line it is on.""" + lines = [ + "schema: ctrlrun.policy/v5", + "actions:", + " stripe.refund:", + ' effect: "refund:{payment_id}"', + f" max_attempts: {value}", + " decision: allow", + ] + return "\n".join(lines) + "\n", lines.index(f" max_attempts: {value}") + 1 + + +@pytest.mark.parametrize("value", ["0", "-1", "true", "1.5", '"3"', "{a: 1}", "[1]", "null"]) +def test_T244_a_malformed_ceiling_is_refused_at_load_naming_key_action_and_line(value): + document, line = _document_with(value) + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document, source="ctrlrun.yaml") + message = str(refused.value) + assert "max_attempts" in message + assert "stripe.refund" in message + assert f"line {line}" in message + + +def test_T244_a_ceiling_needs_schema_v5(): + document = """ +schema: ctrlrun.policy/v4 +actions: + stripe.refund: + effect: "refund:{payment_id}" + max_attempts: 3 + decision: allow +""" + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document, source="ctrlrun.yaml") + message = str(refused.value) + assert "max_attempts" in message + assert "ctrlrun.policy/v5" in message + + +def test_T244_v5_is_a_superset_of_every_earlier_version(): + """Three equality gates in the loader would refuse exactly this (§5.3).""" + document = """ +schema: ctrlrun.policy/v5 +version: "2026-09-11" +environment: production +mode: enforce +controls: + card-data-handling: + title: Card data handling +authority: + grants: + - id: g1 + subject: { agent: "*" } + actions: ["**"] + resources: ["**"] +actions: + stripe.refund: + effect: "refund:{payment_id}" + resource: "payment/{payment_id}" + max_attempts: 3 + controls: [card-data-handling] + data: + payment_id: pii + rules: + - when: { amount_lte: 50000 } + decision: allow + - decision: deny +""" + policy = Policy.from_yaml(document, source="ctrlrun.yaml") + assert policy.max_attempts("stripe.refund") == 3 + assert policy.version == "2026-09-11" + assert policy.environment == "production" + + +def test_T244_a_standalone_authority_document_labelled_v5_is_accepted(): + from ctrlrun.authority import Authority + + document = """ +schema: ctrlrun.policy/v5 +authority: + grants: + - id: g1 + subject: { agent: "*" } + actions: ["**"] + resources: ["**"] +""" + authority = Authority.from_yaml(document, source="authority.yaml", standalone=True) + assert authority is not None + + +def test_T244_two_documents_differing_only_in_a_ceiling_hash_differently(): + three = Policy.from_yaml(ALLOW_CEILING_3, source="ctrlrun.yaml") + one = Policy.from_yaml(ALLOW_CEILING_1, source="ctrlrun.yaml") + assert three.policy_hash != one.policy_hash + + +def test_T244_an_absent_ceiling_reads_as_None(): + assert ( + Policy.from_yaml(ALLOW_NO_CEILING, source="ctrlrun.yaml").max_attempts("stripe.refund") + is None + ) + assert Policy.from_yaml(ALLOW_CEILING_3, source="ctrlrun.yaml").max_attempts("nope") is None + + +# --- T245: the check on the assigned number, alone, through the public route ------------ + + +def _reach_the_check(control, store): + """§5.5's public route to the check, with the fast path live and no seam. + + Attempts 1 and 2 raise `NotExecuted`; attempt 3 raises `TimeoutError`, so the record is + `AMBIGUOUS` at 3 -- which the fast path is normatively not allowed to refuse. Attempt 4 + carries a `reconcile` hook answering `not_executed`: the hook moves the record to `FAILED` + at 3 and the second take renews it to 4, which only the check after the reservation can + refuse. + """ + remote = Remote(_not_executed(), _not_executed(), TimeoutError("the response was lost")) + for _ in range(2): + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(TimeoutError): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + with pytest.raises(ActionDenied) as refused: + _call(control, remote, reconcile=lambda key: "not_executed") + assert refused.value.reason == CEILING + return remote + + +def test_T245_the_check_refuses_the_attempt_the_store_assigned(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = _reach_the_check(control, store) + + assert remote.calls == 3 + + types = [str(event.type) for event in store.events()] + reserved = len(types) - 1 - types[::-1].index("EFFECT_RESERVED") + refused_at = len(types) - 1 - types[::-1].index("EFFECT_RESERVATION_REFUSED") + assert reserved < refused_at, "the check runs after a reservation the store granted" + + refusal = _refusals(store)[-1] + assert refusal.data["reason"] == CEILING + assert refusal.data["attempt"] == 4 + assert refusal.data["max_attempts"] == 3 + + record = store.get_effect(KEY) + assert record.state is EffectState.FAILED + assert record.attempt == 4, "the store assigned 4 and the check refused that number" + + assert _receipts(store)[-1].result is ReceiptResult.BLOCKED + assert "EXECUTION_STARTED" not in types[reserved:] + + +def test_T245_the_fast_path_lets_an_ambiguous_record_through(stores, fake_clock): + """The fast path refuses only a `FAILED` record, normatively (§5.5). + + T245's route and G15 both rest on it: a fast path that refused an `AMBIGUOUS` record at or + above the ceiling would take the check's own test away from it. + """ + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + remote = Remote(TimeoutError("the response was lost"), "ok") + with pytest.raises(TimeoutError): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + with pytest.raises(AmbiguousEffect): + _call(control, remote) + assert _refusals(store)[-1].data["reason"] == "ambiguous" + + +def test_T245_two_callers_that_both_read_below_the_ceiling_do_not_both_get_through( + stores, fake_clock +): + """The concurrency case, deterministic, with the window opened on purpose (§5.5). + + A ceiling of 2 and a record `FAILED` at 1. This caller's fast path reads that record and + passes it, because attempt 2 is within the ceiling. Between that read and the reservation + somebody else renews to 2, dispatches and fails. The store then assigns **3**, and only the + check on the assigned number can refuse it: a kernel whose ceiling is the read alone lets + this attempt execute, which is the attribution-instead-of-prevention §5.5 rejects. + """ + store = stores() + control = _control(ALLOW_CEILING_2, store, fake_clock) + mine = Remote(_not_executed(), "ok") + with pytest.raises(NotExecuted): + _call(control, mine) + assert store.get_effect(KEY).attempt == 1 + + other = Remote(_not_executed()) + with ( + _stale_read(store, lambda: _renew_and_fail(control, other)) as window, + pytest.raises(ActionDenied) as refused, + ): + _call(control, mine) + assert not window["armed"], "the window never opened, so this test proved nothing" + assert refused.value.reason == CEILING + assert mine.calls == 1 and other.calls == 1, "exactly two dispatches under a ceiling of 2" + + refusal = _refusals(store)[-1] + assert refusal.data["attempt"] == 3 + assert refusal.data["max_attempts"] == 2 + record = store.get_effect(KEY) + assert record.state is EffectState.FAILED + assert record.attempt == 3 + + +# --- T245b: the fast path, alone, by the evidence it leaves ----------------------------- + + +def test_T245b_the_fast_path_creates_no_approval_request(stores, fake_clock): + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + remote = Remote(_not_executed()) + approval = _grant(control, store) + with pytest.raises(NotExecuted): + _call(control, remote, approval=approval) + assert store.get_effect(KEY).attempt == 1 + requested_before = len(_events(store, EventType.APPROVAL_REQUESTED)) + + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + assert refused.value.reason == CEILING + assert len(_events(store, EventType.APPROVAL_REQUESTED)) == requested_before, ( + "the fast path runs before the approval gate, so no human is asked" + ) + assert _refusals(store)[-1].data["reason"] == CEILING + assert remote.calls == 1 + + +def test_T245b_the_fast_path_leaves_a_presented_approval_granted(stores, fake_clock): + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + remote = Remote(_not_executed()) + with pytest.raises(NotExecuted): + _call(control, remote, approval=_grant(control, store)) + + second = _grant(control, store) + with pytest.raises(ActionDenied) as refused: + _call(control, remote, approval=second) + assert refused.value.reason == CEILING + assert store.get_approval(second).status == "granted" + assert _refusals(store)[-1].data["reason"] == CEILING + + +def test_T245b_the_fast_path_reserves_nothing(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + remote = Remote(_not_executed()) + with pytest.raises(NotExecuted): + _call(control, remote) + reserved_before = len(_events(store, EventType.EFFECT_RESERVED)) + + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + assert refused.value.reason == CEILING + assert len(_events(store, EventType.EFFECT_RESERVED)) == reserved_before + assert store.get_effect(KEY).attempt == 1 + assert _refusals(store)[-1].data["reason"] == CEILING + + +def test_T245b_without_the_fast_path_the_three_do_not_fail_alike(stores, fake_clock): + """What the fast path is worth, stated as the difference it makes (§8.4 T245b). + + With the fast path deleted, call 1 is not refused at all: the check runs only after an + approval is presented, so an `APPROVE` action with nothing presented raises + `ApprovalRequired` and creates a request. That asymmetry is what the three assertions + above are for, and asserting it here keeps them from reading as one fact repeated. + """ + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + remote = Remote(_not_executed()) + with pytest.raises(NotExecuted): + _call(control, remote, approval=_grant(control, store)) + + # The check alone cannot see this call: it is refused before any approval exists. + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + assert refused.value.reason == CEILING + assert not isinstance(refused.value, ApprovalRequired) + + +# --- T247: both backends, and the v0.6 multi-process standard on Postgres --------------- + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + +postgres = pytest.mark.skipif( + not POSTGRES_URL, reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against" +) + +#: The tree under test: a child that imported a different checkout grades a different kernel. +REPO_SRC = str(Path(__file__).resolve().parents[1] / "src") + +#: Generous, because a bound that fires is a red test and not a hang. +BOUND = 180.0 + +CHILD = textwrap.dedent(""" + import json, os, sys + job = json.loads(sys.stdin.read()) + sys.path.insert(0, job["src"]) + import ctrlrun + _WHERE = os.path.realpath(ctrlrun.__file__) + assert _WHERE.startswith(os.path.realpath(job["src"])), ( + "the child imported ctrlrun from %s, not the tree under test" % _WHERE) + from ctrlrun import ActionDenied, Control, NotExecuted, Policy, context, protect + from ctrlrun.errors import CTRLRunError + from ctrlrun.postgres import PostgresStateStore + + store = PostgresStateStore(job["url"], schema=job["schema"]) + control = Control(Policy.from_yaml(job["policy"]), store) + dispatches = [] + + @protect("stripe.refund", effect="refund:{payment_id}", control=control) + def refund(payment_id, amount): + dispatches.append(1) + raise NotExecuted("the remote rejected it before doing anything") + + refused = None + for _ in range(job["rounds"]): + try: + with context(agent="refund-agent"): + refund(payment_id=job["payment_id"], amount=200) + except NotExecuted: + continue + except ActionDenied as denied: + refused = denied.reason + break + except CTRLRunError: + continue + record = store.get_effect("refund:" + job["payment_id"]) + store.close() + print(json.dumps({ + "dispatches": len(dispatches), + "refused": refused, + "attempt": None if record is None else record.attempt, + "state": None if record is None else str(record.state), + })) +""") + + +@postgres +def test_T247_no_more_than_N_dispatches_across_separate_processes(): + """`v0.6`'s multi-process standard: separate OS processes, one Postgres, one key. + + It depends on item 3a and cannot see item 3a missing: racing processes do not reliably + stall between a `SELECT` and an `UPDATE`, which is why item 3a's T246 opens that window + deterministically and this test does not claim to. What it does claim is the property + §5.5 states: whatever the concurrency, at most `max_attempts` attempts execute. + """ + from ctrlrun.postgres import PostgresStateStore + + # A scratch schema of this test's own, dropped afterwards (SPEC-v0.6 §4.1): a store opened + # on the default `public` would leave tables behind in the operator's own schema, and + # T154f is the test that says so. + schema = f"cap_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + payment_id = f"txn_{uuid.uuid4().hex[:12]}" + job = { + "src": REPO_SRC, + "url": POSTGRES_URL, + "schema": schema, + "policy": ALLOW_CEILING_3, + "payment_id": payment_id, + "rounds": 8, + } + children = [ + subprocess.Popen( + [sys.executable, "-c", CHILD], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + for _ in range(6) + ] + results = [] + try: + # **Every child is fed before any is waited on.** A `communicate()` per child in turn + # feeds the second only once the first has exited, which serialises them: an earlier + # version of this test did that and reported six processes racing where none did. + for child in children: + assert child.stdin is not None + child.stdin.write(json.dumps(job)) + child.stdin.close() + for child in children: + # Bounded, so a child that wedges fails red rather than hanging CI: a timeout is + # not a test failure (SPEC-v0.4 §3.6), it is this test failing to say anything. + child.wait(timeout=BOUND) + assert child.stdout is not None and child.stderr is not None + out, err = child.stdout.read(), child.stderr.read() + assert child.returncode == 0, err + results.append(json.loads(out.strip().splitlines()[-1])) + finally: + for child in children: + if child.poll() is None: # pragma: no cover - only on a wedged child + child.kill() + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + total = sum(result["dispatches"] for result in results) + assert total <= 3, f"the ceiling of 3 admitted {total} dispatches: {results}" + assert total >= 1, "no process reached the executor at all, so nothing was bounded" + assert any(result["refused"] == CEILING for result in results), ( + "no process met the ceiling, so this run did not exercise it" + ) + # The record itself may sit at 4: where the check refuses, the store assigned that number + # and the record is released `FAILED` at it, having executed nothing (§5.5). What is bounded + # is the number of **dispatches**, which is what the guarantee is about. + assert all(result["refused"] in (None, CEILING) for result in results), ( + f"a process was refused for a reason this run cannot explain: {results}" + ) + + +# --- T248: what happens to an approval on a refused attempt ----------------------------- + + +def test_T248_the_check_leaves_the_approval_consumed_and_the_receipt_names_it(stores, fake_clock): + store = stores() + control = _control(APPROVE_CEILING_2, store, fake_clock) + remote = Remote(_not_executed(), TimeoutError("the response was lost")) + with pytest.raises(NotExecuted): + _call(control, remote, approval=_grant(control, store)) + with pytest.raises(TimeoutError): + _call(control, remote, approval=_grant(control, store)) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + third = _grant(control, store) + with pytest.raises(ActionDenied) as refused: + _call(control, remote, approval=third, reconcile=lambda key: "not_executed") + assert refused.value.reason == CEILING + + assert store.get_approval(third).status == "consumed" + blocked = _receipts(store)[-1] + assert blocked.result is ReceiptResult.BLOCKED + assert blocked.approval_id == third + assert remote.calls == 2 + + +# --- T249: a crash between the reservation and the release ------------------------------ + + +class _Killed(BaseException): + """What a `kill -9` looks like from inside the process that is about to stop.""" + + +def _killed(self, *args, **kwargs): + raise _Killed("the process was killed between the reservation and the release") + + +def test_T249_a_crash_between_the_reservation_and_the_release_is_ambiguous( + stores, fake_clock, monkeypatch +): + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + + # §5.5's public route to the check, so no seam is needed to get past the fast path: + # attempts 1 and 2 fail, attempt 3 times out and leaves the record AMBIGUOUS at 3. + remote = Remote(_not_executed(), _not_executed(), TimeoutError("the response was lost")) + for _ in range(2): + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(TimeoutError): + _call(control, remote) + + # The process stops exactly where §5.2 asks about: after the reservation the store + # granted, and before `fail_effect` released it. + monkeypatch.setattr(type(store), "fail_effect", _killed) + with pytest.raises(_Killed): + _call(control, remote, reconcile=lambda key: "not_executed") + record = store.get_effect(KEY) + assert record.state in (EffectState.RESERVED, EffectState.EXECUTING) + assert record.attempt == 4 + assert remote.calls == 3, "the executor was never called for the refused attempt" + + monkeypatch.undo() + fake_clock.advance(timedelta(minutes=10)) + with pytest.raises(AmbiguousEffect): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + +def test_T249_a_record_that_moved_while_the_ceiling_decided_propagates_the_stores_refusal( + stores, fake_clock +): + """§5.7: the refusal propagates after the `blocked` receipt, as `v0.1 §5.5` has it. + + The window is real and it is opened here rather than described: between the reservation the + check refuses and the `begin_execution` that releases it, this attempt's lease lapses and + another attempt declares the effect `AMBIGUOUS` (`v0.1 §5.3 E3`). The store then refuses the + release for the reason it always would, and the caller is told the truer thing, which is + that the key is not theirs any more. + """ + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = Remote(_not_executed(), _not_executed(), TimeoutError("the response was lost")) + for _ in range(2): + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(TimeoutError): + _call(control, remote) + + # Another deployment of the same action, with no ceiling, so it is free to meet the lapsed + # lease and move the record where E3 says it goes. + other = _control(ALLOW_NO_CEILING, store, fake_clock) + + def lease_lapses_and_somebody_ambiguates(): + fake_clock.advance(timedelta(minutes=10)) + with pytest.raises(AmbiguousEffect): + _call(other, Remote("never reached")) + + with ( + _window(store, "reserve_effect", lease_lapses_and_somebody_ambiguates) as window, + pytest.raises(AmbiguousEffect), + ): + _call(control, remote, reconcile=lambda key: "not_executed") + assert not window["armed"], "the window never opened, so this test proved nothing" + + assert remote.calls == 3, "the executor was never called for the refused attempt" + blocked = _receipts(store)[-1] + assert blocked.result is ReceiptResult.BLOCKED + assert "max_attempts is 3" in (blocked.error or "") + assert _refusals(store)[-1].data["reason"] == CEILING + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + +# --- T250: observe mode, resume, and resolved attempts ---------------------------------- + + +def test_T250_observe_mode_records_the_ceiling_and_runs(stores, fake_clock): + store = stores() + control = _control(OBSERVE_CEILING_1, store, fake_clock) + remote = Remote(_not_executed(), "ok") + with pytest.raises(NotExecuted): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.FAILED + + assert _call(control, remote) == "re_txn_1-2" + receipt = _receipts(store)[-1] + assert receipt.result is ReceiptResult.OBSERVED + assert receipt.would_have.blocked_reason == CEILING + assert remote.calls == 2, "observe mode suppresses the decision, not the execution" + + # §5.5: `attempt_ceiling` is a member of `BLOCKED_BY_STATE`, so `ctrlrun stats` counts it on + # the "would have been blocked" line and not as a policy denial. A reason left out of that + # set is a rollout report that quietly stops adding up (`v0.3 §6.4`). + from ctrlrun.reporting import stats_document + + counted = stats_document(_receipts(store), mode="observe", boundary=None) + assert counted["would_have_been_blocked"] == 1 + assert counted["blocked_by_reason"] == {CEILING: 1} + assert counted["would_have_been_denied"] == 0 + + +def test_T250_the_observed_fast_path_records_the_ceiling_before_the_approval_gate( + stores, fake_clock +): + """The fast path's own observe-mode half, told apart from the check's (§5.5, §5.7). + + Both defences write the same `blocked_reason`, so a test on an `ALLOW` action passes with + either one deleted, which is the defence-in-depth problem `CONTRIBUTING.md` names. What tells + them apart is the **order**: `_Observation.block` keeps the first reason, the fast path runs + before the approval gate, and with nothing presented the gate's own reason is + `approval_required`. So on an `APPROVE` action with no approval presented, `attempt_ceiling` + can only have come from the fast path. + """ + document = OBSERVE_CEILING_1.replace("decision: allow", "decision: approve") + store = stores() + control = _control(document, store, fake_clock) + remote = Remote(_not_executed(), "ok") + with pytest.raises(NotExecuted): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.FAILED + + assert _call(control, remote) == "re_txn_1-2" + receipt = _receipts(store)[-1] + assert receipt.result is ReceiptResult.OBSERVED + assert receipt.would_have.blocked_reason == CEILING, ( + "the fast path decided first, as it does in enforce mode" + ) + + +def test_T250_the_observed_check_after_the_reservation_records_the_ceiling_too(stores, fake_clock): + """The other observe-mode half, through the window the check exists for (§5.7). + + Observe mode never calls the `reconcile` hook (`v0.3 §6.2`), so T245's route cannot reach + the check here. What reaches it is the race the check is *for*: a fast path that read a + record below the ceiling, and a reservation that lands after somebody else renewed. + """ + store = stores() + control = _control(OBSERVE_CEILING_2, store, fake_clock) + remote = Remote(_not_executed(), "ok") + with pytest.raises(NotExecuted): + _call(control, remote) + + other = Remote(_not_executed()) + with _stale_read(store, lambda: _renew_and_fail(control, other)): + assert _call(control, remote) == "re_txn_1-2" + receipt = _receipts(store)[-1] + assert receipt.result is ReceiptResult.OBSERVED + assert receipt.would_have.blocked_reason == CEILING + assert store.get_effect(KEY).attempt == 3 + + +def test_T250_a_resumed_leg_past_the_ceiling_is_not_refused(stores, fake_clock): + """`Control.resume` reserves nothing, so there is no new number to compare (§5.7). + + Reached by lowering the ceiling while an attempt is suspended, which is the only way a + resumed leg can be past one: `execute` refuses before it can suspend such an attempt. + """ + store = stores() + control = _control(ALLOW_CEILING_2, store, fake_clock) + with pytest.raises(NotExecuted): + _call(control, Remote(_not_executed())) + + @protect("stripe.refund", effect="refund:{payment_id}", control=control) + def suspending(payment_id: str, amount: int) -> str: + raise Suspended("opaque-state-from-the-server") + + with context(agent="refund-agent"), pytest.raises(Suspended): + suspending(payment_id="txn_1", amount=200) + assert store.get_effect(KEY).attempt == 2 + + lowered = _control(ALLOW_CEILING_1, store, fake_clock) + receipt = lowered.resume("opaque-state-from-the-server", lambda: "re_txn_1") + assert receipt.result is ReceiptResult.COMMITTED + assert store.get_effect(KEY).state is EffectState.COMMITTED + + +def test_T250_an_attempt_a_human_resolved_to_failed_still_counts(stores, fake_clock): + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + remote = Remote(TimeoutError("the response was lost"), "ok") + with pytest.raises(TimeoutError): + _call(control, remote) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + # What `ctrlrun resolve failed` does. + store.resolve_effect(KEY, EffectState.FAILED, "human:ops@example.com") + assert store.get_effect(KEY).attempt == 1 + + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + assert refused.value.reason == CEILING + assert remote.calls == 1 + + +# --- T251: the amendment is in SPEC-v0.1.md --------------------------------------------- + + +def test_T251_the_amendment_is_in_spec_v0_1(): + spec = (Path(__file__).resolve().parents[1] / "docs" / "SPEC-v0.1.md").read_text( + encoding="utf-8" + ) + section = spec.split("### 5.4 Retry rules", 1)[1].split("### 5.5", 1)[0] + # The original table, unchanged, and the amendment beneath it. + original = "| `FAILED` | **allowed** — new attempt, same key, `attempt += 1` | — |" + assert original in section + amendment = section.index("**Amendment (v0.7, `SPEC-v0.7.md` §5).**") + assert amendment > section.index(original) + assert "max_attempts" in section[amendment:] + assert 'ActionDenied(reason="attempt_ceiling")' in section[amendment:] + assert "ctrlrun.policy/v5" in section[amendment:] + + +# --- one warning where a ceiling can count nothing -------------------------------------- + + +def test_a_ceiling_on_an_action_that_resolves_no_effect_key_warns_once(stores, fake_clock, caplog): + """§5.3: the template may come from the decorator, so the policy cannot refuse this.""" + document = """ +schema: ctrlrun.policy/v5 +actions: + stripe.refund: + max_attempts: 3 + decision: allow +""" + store = stores() + control = _control(document, store, fake_clock) + + @protect("stripe.refund", control=control) + def refund(payment_id: str, amount: int) -> str: + return "ok" + + with caplog.at_level("WARNING", logger="ctrlrun"), context(agent="refund-agent"): + for _ in range(3): + refund(payment_id="txn_1", amount=200) + named = [record for record in caplog.records if "max_attempts" in record.getMessage()] + assert len(named) == 1 + assert "stripe.refund" in named[0].getMessage() + + +# --- the derivation of the number the ceiling is compared against ----------------------- + + +def test_the_ceiling_counts_executions_and_not_retries(stores, fake_clock): + """`max_attempts: 1` means no renewal at all, the first attempt included (§5.3).""" + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + remote = Remote(_not_executed()) + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(ActionDenied) as refused: + _call(control, remote) + assert refused.value.reason == CEILING + assert remote.calls == 1 + + +def test_a_committed_effect_is_still_a_duplicate_and_not_a_ceiling_refusal(stores, fake_clock): + """The ceiling acts on a renewal and never widens what `v0.1 §5.4`'s other rows say.""" + from ctrlrun import DuplicateEffect + + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + remote = Remote("ok", "ok") + _call(control, remote) + with pytest.raises(DuplicateEffect): + _call(control, remote) + assert _refusals(store)[-1].data["reason"] == "duplicate" + + +# --- T252: G15 in verify, and what the ceiling does to G5's selection -------------------- + +#: G15's `N/A` reasons and G5's new one, verbatim from §8.9. +G15_NOT_DECLARED = ( + "no action verify can drive to allow or approve declares both `effect:` and `max_attempts`" +) +G15_ABOVE_BOUND = "every declared max_attempts is above verify's bound of 100 attempts" +CEILING_FORBIDS_RENEWAL = ( + "every action with an `effect:` template that verify can select (a decision of allow or " + "approve under a grant that covers it) declares max_attempts: 1, so no renewal can happen" +) + +V5_CEILING_3 = """ +schema: ctrlrun.policy/v5 +actions: + acme.refund: + effect: "refund:{payment_id}" + max_attempts: 3 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny +""" + +V5_CEILING_1_ONLY = """ +schema: ctrlrun.policy/v5 +actions: + acme.refund: + effect: "refund:{payment_id}" + max_attempts: 1 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny +""" + +V5_NO_CEILING = """ +schema: ctrlrun.policy/v5 +actions: + acme.refund: + effect: "refund:{payment_id}" + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny +""" + +V5_CEILING_ABOVE_BOUND = """ +schema: ctrlrun.policy/v5 +actions: + acme.refund: + effect: "refund:{payment_id}" + max_attempts: 1000 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny +""" + +#: §8.9's precedence case: the ceiling is the only reason nothing is selectable, and the +#: deny-only action B must not make the sentence read as G5's old one. +V5_CAPPED_A_AND_DENIED_B = """ +schema: ctrlrun.policy/v5 +actions: + acme.alpha: + effect: "alpha:{ticket}" + max_attempts: 1 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny + acme.beta: + effect: "beta:{ticket}" + decision: deny +""" + + +def _verify(tmp_path, document, *, only): + from ctrlrun.verify import run as run_verify + + path = tmp_path / "ctrlrun.yaml" + path.write_text(document, encoding="utf-8") + report = run_verify(path, only=only) + return {result.id: result for result in report.guarantees} + + +def test_T252_G15_is_in_the_catalogue(): + from ctrlrun.verify import guarantees as reg + + assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert "G15" in reg.BY_ID + assert "v0.1 §5.4" in reg.BY_ID["G15"].descends_from + + +def test_T252_G15_is_graded_where_the_document_declares_a_ceiling(tmp_path): + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_CEILING_3, only=("G15",))["G15"] + assert result.status is Status.PASS, (result.reason, result.counterexample) + + +def test_T252_G15_is_graded_at_a_ceiling_of_one(tmp_path): + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_CEILING_1_ONLY, only=("G15",))["G15"] + assert result.status is Status.PASS, (result.reason, result.counterexample) + + +def test_T252_G15_is_not_applicable_where_no_action_declares_a_ceiling(tmp_path): + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_NO_CEILING, only=("G15",))["G15"] + assert result.status is Status.NOT_APPLICABLE + assert result.reason == G15_NOT_DECLARED + + +def test_T252_G15_is_not_applicable_above_verifys_bound(tmp_path): + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_CEILING_ABOVE_BOUND, only=("G15",))["G15"] + assert result.status is Status.NOT_APPLICABLE + assert result.reason == G15_ABOVE_BOUND + + +def test_T252_a_kernel_with_the_check_deleted_fails_G15(tmp_path, monkeypatch): + """The control §8.9 asks for: G15 must be able to fail, and on the check's own mechanism. + + An earlier draft drove N+1 sequential `NotExecuted` attempts, which the fast path alone + refuses, so G15 passed with the guarantee's own mechanism deleted. + """ + from ctrlrun.verify import Status + + monkeypatch.setattr(Control, "_over_the_ceiling", lambda self, ceiling, attempt: False) + result = _verify(tmp_path, V5_CEILING_3, only=("G15",))["G15"] + assert result.status is Status.FAIL + assert result.reason != "control failed", "the observable fails, not the control" + + +def test_T252_a_kernel_that_refuses_every_renewal_fails_G15s_control(tmp_path, monkeypatch): + from ctrlrun.verify import Status + + monkeypatch.setattr(Control, "_over_the_ceiling", lambda self, ceiling, attempt: True) + result = _verify(tmp_path, V5_CEILING_3, only=("G15",))["G15"] + assert result.status is Status.FAIL + assert result.reason == "control failed" + + +def test_T252_G5_is_still_graded_where_the_ceiling_allows_a_renewal(tmp_path): + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_CEILING_3, only=("G5",))["G5"] + assert result.status is Status.PASS, (result.reason, result.counterexample) + + +def test_T252_G5_is_not_applicable_where_every_ceiling_forbids_a_renewal(tmp_path): + """`max_attempts: 1` would otherwise turn G5's control into a false `fail` (§8.9).""" + from ctrlrun.verify import Status + + result = _verify(tmp_path, V5_CEILING_1_ONLY, only=("G5",))["G5"] + assert result.status is Status.NOT_APPLICABLE, (result.reason, result.counterexample) + assert result.reason == CEILING_FORBIDS_RENEWAL + + +def test_T252_G5s_ceiling_sentence_is_printed_only_where_the_ceiling_is_the_only_reason(tmp_path): + from ctrlrun.verify import Status + from ctrlrun.verify import guarantees as reg + + result = _verify(tmp_path, V5_CAPPED_A_AND_DENIED_B, only=("G5",))["G5"] + assert result.status is Status.NOT_APPLICABLE + assert result.reason == CEILING_FORBIDS_RENEWAL + + # And where nothing is selectable for the older reason, that reason still wins. + nothing = """ +schema: ctrlrun.policy/v5 +actions: + acme.refund: + max_attempts: 1 + decision: allow +""" + result = _verify(tmp_path, nothing, only=("G5",))["G5"] + assert result.status is Status.NOT_APPLICABLE + assert result.reason == reg.NO_EFFECT_TEMPLATE + + +def test_T252_G5_selects_the_uncapped_action_where_there_is_one(tmp_path): + from ctrlrun.verify import Status + + both = """ +schema: ctrlrun.policy/v5 +actions: + acme.alpha: + effect: "alpha:{ticket}" + max_attempts: 1 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny + acme.beta: + effect: "beta:{ticket}" + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny +""" + result = _verify(tmp_path, both, only=("G5",))["G5"] + assert result.status is Status.PASS, (result.reason, result.counterexample) + assert result.action == "acme.beta", "G5 skips the action whose ceiling forbids a renewal" diff --git a/tests/test_verify.py b/tests/test_verify.py index 86fff22..b273796 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -169,13 +169,14 @@ def test_T101_a_policy_with_no_approve_rule_makes_G1_and_G2_not_applicable(tmp_p assert report.applicable == report.passed + report.failed # Ten in the catalogue; G1 and G2 for the missing approve band, G8 and G9 for the # missing authority section. Six applicable, and the count is over those six. - # And G13, which is N/A on every SQLite run: SQLite has no clock of its own (SPEC-v0.7 §8.9). + # And G13, which is N/A on every SQLite run: SQLite has no clock of its own (SPEC-v0.7 §8.9), + # and G15, because this document names no `max_attempts` (SPEC-v0.7 §8.9). assert report.applicable == 7 - assert report.not_applicable == 5 + assert report.not_applicable == 6 text = report.to_text() assert "8/8" not in text assert f"{report.passed}/{report.applicable} declared guarantees pass." in text - assert "5 not applicable: G1, G2, G8, G9, G13." in text + assert "6 not applicable: G1, G2, G8, G9, G13, G15." in text def test_T101b_zero_applicable_guarantees_is_not_a_pass(tmp_path): @@ -866,11 +867,12 @@ def test_the_v1_payments_template_reports_six_over_six_with_six_not_applicable() report = run(V1_PAYMENTS) assert report.exit_code == 0 - assert (report.passed, report.applicable, report.not_applicable) == (6, 6, 6) + assert (report.passed, report.applicable, report.not_applicable) == (6, 6, 7) text = report.to_text() assert "6/6 declared guarantees pass." in text - # G13 is N/A on SQLite, which has no clock of its own (SPEC-v0.7 §8.9). - assert "6 not applicable: G3, G4, G5, G8, G9, G13." in text + # G13 is N/A on SQLite, which has no clock of its own, and G15 because this template + # names no `max_attempts` (SPEC-v0.7 §8.9). + assert "7 not applicable: G3, G4, G5, G8, G9, G13, G15." in text assert "10/10" not in text diff --git a/tests/test_verify_action.py b/tests/test_verify_action.py index ba8fa80..4a25122 100644 --- a/tests/test_verify_action.py +++ b/tests/test_verify_action.py @@ -139,8 +139,8 @@ def test_T118_ci_asserts_the_two_shapes_the_specification_names(): assert 'test "$AUTHORITY" = "verified 11/11"' in script assert 'test "$TEMPLATES" = "verified 6/6"' in script - assert 'test "$AUTHORITY_NA" = "1"' in script - assert 'test "$TEMPLATES_NA" = "6"' in script + assert 'test "$AUTHORITY_NA" = "2"' in script + assert 'test "$TEMPLATES_NA" = "7"' in script @pytest.mark.authority @@ -153,11 +153,12 @@ def test_T118_the_two_configurations_really_do_report_those_shapes(): assert authority.badge is not None assert authority.badge["message"] == "verified 11/11" - # G13 only: SQLite has no clock of its own to diverge from (SPEC-v0.7 §8.9). - assert authority.not_applicable == 1 + # G13 and G15: SQLite has no clock of its own to diverge from, and the document declares + # no `max_attempts` (SPEC-v0.7 §8.9). + assert authority.not_applicable == 2 assert templates.badge is not None assert templates.badge["message"] == "verified 6/6" - assert templates.not_applicable == 6 + assert templates.not_applicable == 7 def test_T118_the_action_uploads_the_report_and_writes_a_job_summary(): @@ -210,7 +211,7 @@ def test_T119_the_colour_is_about_failures_and_has_no_amber_for_not_applicable( from ctrlrun.verify import scenarios passing = run(V1_PAYMENTS) - assert passing.not_applicable == 6 + assert passing.not_applicable == 7 assert passing.badge is not None assert passing.badge["color"] == BADGE_PASS_COLOR diff --git a/tests/test_verify_report.py b/tests/test_verify_report.py index f7727d6..d239b75 100644 --- a/tests/test_verify_report.py +++ b/tests/test_verify_report.py @@ -106,8 +106,9 @@ def test_T113_the_summary_is_the_last_line_and_names_the_not_applicable_ids(tmp_ assert last == report.summary_line() assert last.startswith(f"{report.passed}/{report.applicable} declared guarantees pass.") - # G13 is N/A on every SQLite run (SPEC-v0.7 §8.9): SQLite has no clock of its own. - assert "6 not applicable: G3, G4, G5, G8, G9, G13." in last + # G13 is N/A on every SQLite run and G15 wherever no action declares a ceiling + # (SPEC-v0.7 §8.9): SQLite has no clock of its own, and this document names no bound. + assert "7 not applicable: G3, G4, G5, G8, G9, G13, G15." in last # The fraction is passes over applicable. A report with six N/As does not say 12/12. assert "12/12" not in text @@ -132,9 +133,9 @@ def test_T113_a_failing_report_names_the_subject_and_prints_the_counterexample( @pytest.mark.parametrize( ("document", "expected"), [ - (ALL_APPLICABLE, "9/9 declared guarantees pass. 3 not applicable"), - (WITH_NOT_APPLICABLE, "6/6 declared guarantees pass. 6 not applicable"), - (EMPTY, "0/0 declared guarantees pass. 12 not applicable"), + (ALL_APPLICABLE, "9/9 declared guarantees pass. 4 not applicable"), + (WITH_NOT_APPLICABLE, "6/6 declared guarantees pass. 7 not applicable"), + (EMPTY, "0/0 declared guarantees pass. 13 not applicable"), ], ids=["passing", "some-na", "all-na"], ) @@ -445,7 +446,7 @@ def test_T116_a_run_with_six_not_applicable_still_exits_0(tmp_path, monkeypatch) result = _cli(tmp_path, WITH_NOT_APPLICABLE) assert result.exit_code == 0 - assert "6 not applicable" in result.stdout + assert "7 not applicable" in result.stdout def test_T116_json_and_junit_can_be_combined(tmp_path, monkeypatch): From ea712396a504e62891e025189bafdf02bbe052db Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 00:14:30 +0530 Subject: [PATCH 2/3] Answer the item 4 review: two blocking, nine smaller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both blocking findings were sentences that were not true, and the fix for each is words rather than code. §5.5 claimed the fast path saves a human from being asked, with one stated exception, and named the wrong one. On the kernel's own primary route, the one T245 and G15 are built on, a record that is AMBIGUOUS goes through the fast path correctly and reaches the approval gate BEFORE any reconcile, so a new approval request is created, a human grants it, and the retry consumes it in the reservation the check then refuses. §5.5 stops claiming exhaustiveness and lists the routes it knows as an open list; §5.2 says a new request can be created and granted on the way to a refusal; a test pins it. Not closed: re-reading the record between the reconcile and the second take would save the approval and would leave the check with no seamless public route, so G15 would grade a guarantee that could not have failed. G15's above-the-bound N/A reason was false of a document with a low ceiling on an action verify cannot drive, which is the identical defect §8.9 had already caught and fixed in the sentence beside it. Reworded to the scope the fallback selection actually has. And: the fast path's refusal records the presented approval (v0.6 §7.2.1, as the DENY path already did); _refuse_ceiling writes the event and the receipt before attempting the release and catches any store refusal, since InvalidArgument was escaping with the evidence; T240 to T245b run on Postgres too; T247 asserts that two children's windows overlap, because every assertion it had was satisfied by six processes running one after another; §5.5 says the refused attempt number is spent; §5.7 says the ceiling bounds attempts and not executor invocations; §7.1 enumerates the ceiling per entry point with the "no" rows written down; Control.evaluate's docstring says it does not see the ceiling; and G15's title fits the report table's width, with a test that keeps every title inside it. --- CHANGELOG.md | 19 +- docs/SPEC-v0.7.md | 170 +++++++++++++++--- src/ctrlrun/control.py | 57 ++++-- src/ctrlrun/verify/guarantees.py | 14 +- src/ctrlrun/verify/scenarios.py | 5 + tests/test_attempt_cap.py | 296 +++++++++++++++++++++++++++++-- 6 files changed, 506 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3f9d9c..24379fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,8 +31,11 @@ any change to one appears here. on SQLite and the in-memory store because neither has a clock of its own. - **The attempt ceiling, `max_attempts`** (SPEC-v0.7 §5, item 4, and the amendment to `docs/SPEC-v0.1.md` §5.4). A new action-entry policy key, an integer of at least 1, bounding the - attempts that may **execute** on one effect key, the first included: `max_attempts: 3` is the - first attempt and two renewals. It needs `schema: ctrlrun.policy/v5`, a new schema version that + **attempts** that may execute on one effect key, the first included: `max_attempts: 3` is the + first attempt and two renewals. **An attempt, not an executor invocation**: a `Suspended` + executor holds its reservation and every `Control.resume` runs on that same attempt, so an + elicitation loop is one dispatch however many rounds it takes. The gateway bounds those with + `max_elicitation_rounds`; a direct `Control.resume` caller has no bound, and this adds none. It needs `schema: ctrlrun.policy/v5`, a new schema version that is a superset of `v4` as `v4` is of `v3`; `0`, a negative, a `bool`, a float, a string and a mapping are each a `PolicyError` at load, naming the key, the action and the line. The ceiling is inside the policy hash, so a receipt records which one refused an attempt. @@ -41,10 +44,14 @@ any change to one appears here. taken before reserving. Above the ceiling the executor is not called, the record is released as `FAILED` with an error naming the ceiling, `EFFECT_RESERVATION_REFUSED` carries `reason: "attempt_ceiling"` with the attempt and the ceiling, a `blocked` receipt is written, - and `ActionDenied(reason="attempt_ceiling")` is raised. A read of the record before the approval - gate refuses the ordinary sequential case earlier, writing nothing, spending no presented - approval and creating no approval request; it refuses only a `FAILED` record and is never the - guarantee. In observe mode the refusal is recorded as `would_have.blocked_reason: + and `ActionDenied(reason="attempt_ceiling")` is raised. The refused attempt number is **spent**: + raising `max_attempts` from 2 to 4 after a refusal buys one further dispatch, not two. A read of + the record before the approval gate refuses the ordinary sequential case earlier, writing + nothing, spending no presented approval and creating no approval request; it refuses only a + `FAILED` record and is never the guarantee. **On any other route the approval gate comes + first**, so on an `APPROVE` action a human can be asked, and answer, for an attempt that is then + refused: a wasted answer, never an execution, and `docs/SPEC-v0.7.md` §5.2 and §5.5 say so + rather than closing it. In observe mode the refusal is recorded as `would_have.blocked_reason: "attempt_ceiling"` and the action runs. Verify gains **G15**, and G5 now selects only an action whose ceiling permits a renewal, reporting `N/A` where the ceiling is the only reason it cannot, because G5's control *is* a renewal and `max_attempts: 1` would otherwise report a correct diff --git a/docs/SPEC-v0.7.md b/docs/SPEC-v0.7.md index 7cb9450..1df39c0 100644 --- a/docs/SPEC-v0.7.md +++ b/docs/SPEC-v0.7.md @@ -908,12 +908,20 @@ action with no approval presented runs anyway (`control.py:821-827`, `899-925`). roadmap, §1.4 item 2.) **What happens to an approval on a refused attempt.** Where the ceiling is found by §5.5's fast path, -before the approval gate, nothing has been written and a presented approval stays `granted`. Where it -is found by the check after the reservation, the approval was consumed in the same transaction that -assigned the attempt number, and **it stays consumed**: the store has no way to un-consume an -approval, `v0.1 §4.2 A2` is that consumption is single-use and atomic, and v0.7 adds no method. What -is lost is bounded: every later attempt on that key is also over the ceiling, so an unspent approval -would open nothing there until the operator raised it. +before the approval gate, nothing has been written and a presented approval stays `granted`; the refusal +records it (`v0.6 §7.2.1`) and does not spend it. Where it is found by the check after the reservation, +the approval was consumed in the same transaction that assigned the attempt number, and **it stays +consumed**: the store has no way to un-consume an approval, `v0.1 §4.2 A2` is that consumption is +single-use and atomic, and v0.7 adds no method. + +**And an approval that did not exist yet can be created and granted on the way to that refusal.** Where +the record is not `FAILED` the fast path answers nothing, so the approval gate is reached first: on §5.5's +reconcile route an `APPROVE` action with nothing presented has a **new** request created and raises +`ApprovalRequired`, a human grants it, and the retry consumes it in the reservation the check then refuses. +So the cost is not only "an already-granted approval stays consumed"; it is that a human can be asked, and +answer, for an attempt that could never run. §5.5 states both, and neither is closed. What is bounded is +what it costs: every later attempt on that key is also over the ceiling, so no approval spent here opens +anything until the operator raises `max_attempts`, and nothing ever executes. **What a crash between the reservation and the release leaves.** The record is `RESERVED`, or `EXECUTING` if the crash fell after `begin_execution`, under a live lease. When the lease lapses the @@ -1004,9 +1012,27 @@ Above the ceiling: 3. **`EFFECT_RESERVATION_REFUSED` is appended** with `data.reason = "attempt_ceiling"`, `data.attempt` and `data.max_attempts`, on the existing event type, so the history says why. 4. **A `blocked` receipt** is written, keeping the decision the policy reached (`v0.1 §4.2 A1`'s - precedent) and carrying the approval where one was consumed. + precedent) and carrying the approval where one was consumed or presented. 5. **`ActionDenied(reason="attempt_ceiling")` is raised.** +**The evidence is written before the release is attempted**, and a store refusal of any type is caught +and re-raised after it. `begin_execution` and `fail_effect` can refuse with `DuplicateEffect` or +`AmbiguousEffect` where the record moved (§5.7), and with `InvalidArgument` where it moved under a +different `action_id` with a dead lease. On any of those the caller gets the store's exception, as +`v0.1 §5.5` requires, and the event and the receipt exist either way: a refusal that took its own +evidence with it would leave a `RESERVED` record nothing in the history explains. + +**The refused number is spent.** The store assigned attempt *n* + 1 before the check could look at it, +and releasing the record `FAILED` does not give it back: the next renewal is *n* + 2. So an operator +who raises `max_attempts` from 2 to 4 after a refusal buys **one** further dispatch, not two, because +attempt 3 is gone. §5.7's reason for counting a resolved attempt is that *it was dispatched*, and this +one was not, which makes the arithmetic worth stating rather than leaving to be discovered. Giving the +number back would mean a second write on the refusal path and a record whose attempt number moves +backwards, which `v0.7 §5.6` spent a whole build item making impossible. The released record's `error` +also carries the ceiling text rather than the last executor outcome, so `ctrlrun effects` shows why the +key stopped rather than what the previous attempt's remote said; the events and the receipts still carry +that, in order. + **Why `ActionDenied`.** It is the only type in the closed set whose meaning is true here: "the action may not run, and `reason` says why", which is what an operator's ceiling says. `DuplicateEffect` would tell the caller the effect happened or is happening, and it did not; the gateway would say @@ -1031,19 +1057,35 @@ already at or above the ceiling, the call is refused the same way. **The fast pa to the reservation, which refuses or reconciles it exactly as at 0.6.1, and T245's route and G15 both depend on that. It refuses with `EFFECT_RESERVATION_REFUSED` (`data.reason = "attempt_ceiling"`), a `blocked` receipt and `ActionDenied(reason="attempt_ceiling")`, with nothing reserved, nothing released and -nothing consumed, and **no approval request is created**. It saves a write, it saves a -presented approval from being spent, and it saves a human from being asked about an attempt that could -never run, which `v0.3 §4.3`'s first reason says a denial must never do. - -**With one exception, stated.** An adapter decides whether to interrupt for a human through -`ctrlrun.adapter.needs_approval`, which calls `Control.evaluate` (`adapter.py:408-450`), and -`Control.evaluate` does not see the ceiling: it takes an `Action`, not an effect key, and it writes -nothing. So a framework can put an approval in front of a human for an attempt the fast path then -refuses; the human's answer is recorded, the approval is left `granted` because the fast path consumes -nothing, and nothing runs. Teaching `evaluate` the ceiling would mean resolving an effect template and -reading the store inside a method whose contract is a decision about an action, and that is not this -milestone's change to make. The cost is one wasted answer per refused attempt, and §1.4 item 2 -records it. +nothing consumed, and **no approval request is created**. A presented approval is recorded against the +refusal it met, on `v0.6 §7.2.1`'s rule, and is not spent. It saves a write, it saves a +presented approval from being spent, and **on the sequential `FAILED` route** it saves a human from being +asked about an attempt that could never run, which `v0.3 §4.3`'s first reason says a denial must never do. + +**On the other routes a human is still asked, and this list is not exhaustive.** The fast path answers +only for a record that is already `FAILED` at the ceiling. Wherever the record is in any other state, the +approval gate is reached first and the saving above does not apply. Two such routes are known, and a +reader should assume there are others rather than read this as a closed set: + +- **The reconcile route, which is the kernel's own primary public route** and the one T245 and G15 are + built on. Attempt N ends `AMBIGUOUS`; attempt N+1 carries a `reconcile` hook and presents nothing; the + fast path correctly lets the `AMBIGUOUS` record through; `_secure` reaches `_presented` **before** any + reconcile runs (`control.py`'s `_secure`, first `_take`), so a **new** approval request is created and + `ApprovalRequired` is raised. A human grants it, the agent retries, the hook moves the record to `FAILED` + at N, the second `_take` renews to N+1 **and consumes the approval**, and only then does the check refuse. + One human answer spent, and nothing run. **This is stated and not closed**: re-reading the record between + the reconcile and the second `_take` would save the approval and would destroy "the check alone is + reachable through a public route, with no seam" below, leaving the check with no seamless route and G15 + with nothing to grade it through. The cost is one wasted answer; the alternative is an unexercised + guarantee, and `v0.4 §1.3`'s rule is that a guarantee that could not have failed is not a pass. +- **`Control.evaluate` does not see the ceiling** (§7). An adapter decides whether to interrupt for a human + through `ctrlrun.adapter.needs_approval`, which calls it (`adapter.py:408-450`); it takes an `Action`, not + an effect key, and it writes nothing, so it cannot resolve a record to count on. A framework can therefore + put an approval in front of a human for an attempt `execute` then refuses. Teaching `evaluate` the ceiling + would mean resolving an effect template and reading the store inside a method whose contract is a decision + about an action, and that is not this milestone's change to make. + +§1.4 item 2's cost line covers both: **one wasted answer per refused attempt**, never an execution. It is a fast path and **never the guarantee**. Two callers who both read attempt N−1 both pass it, and only the check on the assigned number stops the second. The two defences are independent, so each gets @@ -1166,6 +1208,14 @@ and if that holds here §12 says so and T246 and T246b are the only tests of the another, not that it did not happen at the provider's door. An operator who wants more raises `max_attempts`, which the policy hash records. - **`Control.resume` is untouched.** It reserves nothing, so there is no new number to compare. + **The consequence, stated: `max_attempts` bounds attempts, not executor invocations.** One attempt can + invoke the executor many times, because a `Suspended` executor holds its reservation and every `resume` + runs on that same attempt (`v0.2 §6.9`). Under `max_attempts: 1` an executor that suspends every round + can be resumed indefinitely, with the approval consumed once and the record still `EXECUTING` at attempt + 1. That is not a hole in the ceiling: an elicitation round is a continuation of one dispatch, not a + second one, and what a ceiling exists to bound is dispatches. It is bounded where it matters by + `max_elicitation_rounds` on the gateway (`v0.2 §6.9.2`); **a direct `Control.resume` caller has no such + bound, and v0.7 does not add one.** - **Observe mode records and runs.** The fast path and the check record `attempt_ceiling` in `would_have.blocked_reason` and the action executes, because observe mode suppresses CTRLRun's decisions and not the record of an effect that happened (`v0.3 §6.2`, `v0.6 §7.2.3`). @@ -1586,6 +1636,35 @@ request whose fingerprint already exists, and consume nothing. **`v0.3 §4.3.1`'s order** is amended as §5.5 states, and the new column is recorded there by item 5, in the same commit as the code. +### 7.1 The same column for the attempt ceiling + +Item 4 amends `v0.3 §4.3.1`'s **order** (§5.5) and so it owes the same enumeration, for the reason +`v0.3 §4.3.1` exists at all: `Control.delegate` let an expired credential mint permanent authority not +because a check was wrong but because nothing listed the other paths. The "no" rows are written down as +deliberately as the "yes" ones, and two of them are where this milestone's review found its findings. + +| Entry point | Applies the ceiling | Why | +|---|---|---| +| `@protect` → `Control.execute` | **yes**, both defences: the fast path before the approval gate, and the check on the assigned attempt number before the executor | the only path that reserves and then dispatches (§5.5) | +| `Control.execute` called directly | **yes**, the same two | the same method | +| `Control.execute` in observe mode | **records, does not enforce**: both defences write `would_have.blocked_reason = "attempt_ceiling"` and the action runs | `v0.3 §6.2`: observe mode suppresses CTRLRun's decisions, not the record of an effect that happened | +| `Control.evaluate` | **no** | it takes an `Action` and not an effect key, and it writes nothing, so it can resolve no record to count on. A caller can therefore be told `approve` for an attempt `execute` will refuse (§5.5). Its docstring says so | +| `Control.resume` | **no** | it reserves nothing, so there is no new number to compare. One attempt can invoke the executor many times through it, which §5.7 states | +| `Control.delegate` / `Control.revoke` | **no** | they reserve no effect | +| The gateway's `tools/call` | **yes**, through `Control.execute`; `ActionDenied(reason="attempt_ceiling")` maps to `-41001` with the reason in the error data, unchanged | it is `Control.execute` behind a transport (§5.5) | +| The gateway's approval pre-check | **no** | it reads `Control.evaluate`, and inherits that row exactly | +| `ctrlrun.acs`'s request hook | **yes**, through `Control.execute`; the refusal becomes `deny` with the reason in `codes`, unchanged | the same shape | +| An adapter's protected tool → `@protect` → `Control.execute` | **yes**, the same two | the `@protect` row reached through a framework (`v0.5 §4.1`) | +| `ctrlrun.adapter.needs_approval` → `Control.evaluate` | **no** | `Control.evaluate`'s row; it can interrupt for a human on an attempt that will then be refused (§5.5) | +| `ctrlrun.verify.run` | drives the first row | G15 grades the check through §5.5's public route, and G5 and G14 select only where a ceiling permits a renewal (§8.9) | +| `ctrlrun approve` / `ctrlrun deny` / `WebhookApprovalProvider`'s callback / the operator server's write tools | **no** | they record an answer to a request; nothing reserves, and `Control.execute` applies the ceiling when the answer is presented | +| `ctrlrun resolve` → `resolve_effect` | **no**, and the attempt it resolved still counts | it moves a record out of `AMBIGUOUS` and reserves nothing; §5.7 argues why a resolved attempt counts | + +**No row reserves without passing through the check**: `reserve_effect` and `consume_approval_and_reserve` +are called from `Control._take` and `Control._observe_take` and from nowhere else in the package, and both +are reached only from `Control.execute` (enforce) and `Control._observed` (observe), which are the first +three rows. + --- ## 8. Acceptance tests @@ -2379,7 +2458,9 @@ Each in the item that makes it true, and each recorded here so it can be found. 2. **`v0.1 §6.2`'s event list** gains `CLOCK_SKEW_DETECTED` (item 1). 3. **`v0.2 §6.8`'s transport rows** are unchanged in meaning and now implemented by `ctrlrun.transport`; the gateway's `NotExecuted` is chained (item 2). -4. **`v0.3 §4.3.1`** gains §7's column and §5.5's order (items 4 and 5). +4. **`v0.3 §4.3.1`** gains two columns and one reordering: §7's precondition column and §7.1's attempt + ceiling column, and §5.5's order (items 4 and 5). Item 4 owes §7.1 because it amends the order, and + because a missing enumeration is how `Control.delegate`'s hole arrived. 5. **`v0.4 §3.7`** becomes *verify opens no connection except to the store `--store-url` names and to loopback listeners it bound itself*, and T107's guard admits only the `127.0.0.1` ports the run bound (item 2). **G5's selection** (`v0.4 §2.2`) skips an action whose ceiling forbids a renewal (item 4, §8.9). @@ -2911,7 +2992,52 @@ for a different reason. Postgres under a ceiling of three. Racing processes do not reliably stall between a `SELECT` and an `UPDATE`, so the test cannot claim to open item 3a's window and does not: it asserts that the total number of executor calls never exceeds the ceiling, that at least one was made, and that at least one process was refused with -`attempt_ceiling`, so a run in which nothing contended fails rather than passing quietly. +`attempt_ceiling`. **The independent review measured what that is worth**, and the numbers belong here rather +than in a claim: the children start within 8 ms of each other with overlapping windows, and with the check +deleted T247 goes red on 3 runs in 12, with the fast path deleted on 0 in 10. So T247 grades the property and +`T245`'s deterministic window grades the check. The review also showed the first version of this paragraph was +false: every assertion it listed is satisfied by six processes running one after another, which is what the test +did before the feed-all-then-wait loop, and the reviewer proved it by putting the serialisation back and watching +it pass. T247 now asserts that at least two children's execution windows **overlap**, which is the one thing a +serialised run cannot produce; the serialisation mutant is red 3 runs in 3 against it. + +**The review's two blocking findings were both sentences that were not true, and neither was fixed with code.** + +*§5.5 claimed the fast path saves a human from being asked, with one stated exception, and the exception was the +wrong one.* The reviewer reproduced the real case on the kernel's own primary route, not the adapter's: under +`max_attempts: 1` with `decision: approve`, attempt 1 times out, attempt 2 carries a `reconcile` hook and +presents nothing, the fast path correctly lets the `AMBIGUOUS` record through, and `_secure` reaches `_presented` +**before** any reconcile runs. A new approval request is created, a human grants it, the retry consumes it in the +reservation the check then refuses. One dispatch under a ceiling of 1 and one human answer spent. **Not closed, +deliberately.** Re-reading the record between `_reconciled(...)` and the second `_take` would save the approval +and would destroy "the check alone is reachable through a public route, with no seam", which is the only route +T245 and G15 have to the check: a guarantee that could not have failed is not a pass (`v0.4 §1.3`). So §5.5 stops +claiming exhaustiveness and lists the routes it knows with a warning that the list is not closed, §5.2 says a +**new** request can be created and granted on the way to a refusal rather than only that an existing one stays +consumed, and a test pins the behaviour so the claim cannot drift back. + +*G15's `CEILING_ABOVE_BOUND` was false of a document with a low ceiling on an action verify cannot drive.* The +fallback selection re-applies the effect and ceiling filters and drops only the bound, so it can only ever +describe the selectable actions, while the sentence said "every declared". A deny-only action with +`max_attempts: 3` made it a lie. It is now worded on `NO_CEILING_DECLARED`'s shape, scoped to what verify can +drive, which is the identical fix §8.9 had already made to the sibling sentence: **the same defect, in the +sentence written next to the one that documented it.** + +**Nine smaller findings, and what each changed.** The fast path's refusal now records the presented approval, on +`v0.6 §7.2.1`'s rule, which a previous review had already applied to the `DENY` path and which this path had +missed the same way. `_refuse_ceiling` writes the event and the receipt **before** attempting the release and +catches any `CTRLRunError` from it, because `_checked` raises `InvalidArgument` for a record that moved under a +different `action_id` with a dead lease, and that escaped before either was written, leaving a `RESERVED` record +nothing explained. T240 to T245b run on Postgres as well as SQLite and in-memory, which §8.4's first sentence +asked for and the fixture did not do; the positive control drops `CLOCK_SKEW_DETECTED` from its comparison, +because every Postgres scratch store here is opened with a frozen clock and truthfully reports days of skew +(§1.4 item 6). §5.5 says the refused number is spent and what it costs an operator who later raises the ceiling; +§5.7 says `max_attempts` bounds attempts and not executor invocations, since a `Suspended` executor can be +resumed indefinitely on one attempt and only the gateway's `max_elicitation_rounds` bounds that; §7.1 is the +per-entry-point enumeration item 4 owed for amending `v0.3 §4.3.1`'s order, with every "no" row written down, +and `Control.evaluate`'s docstring now says it does not see the ceiling. G15's title was 48 characters against +`report._TITLE_WIDTH`'s 32, the only one over, so it is "renewal past the ceiling refused" and a test asserts +no title ever exceeds the width again. ### 12.5 Item 5: precondition fingerprints diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index de016d7..e3e57c8 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -416,6 +416,13 @@ def evaluate(self, action: Action) -> Evaluation: would otherwise have no meaning on the one path that may not have them. The gateway's approval pre-check reads this, so leaving it undefined would give three different gateway behaviours. + + **It does not see the attempt ceiling** (SPEC-v0.7 §5.5, §7). `max_attempts` is decided + against an effect record, and this method takes an `Action` rather than an effect key and + may not read the store to resolve one. So an adapter asking `ctrlrun.adapter.needs_approval` + can put an approval in front of a human for an attempt `execute` will then refuse, and a + gateway pre-check can report `approve` for the same attempt. The cost is a wasted answer, + never an execution: nothing here writes, and every ceiling refusal happens in `execute`. """ # SPEC-v0.3 §4.3.1 — the environment obeys §2.5 on *every* row of that table, and # `evaluate` is one. Read-only, so this refuses rather than denies: an Action from @@ -743,6 +750,11 @@ def execute( # race that enforce mode does not have. observation.block(BLOCKED_ATTEMPT_CEILING) else: + # SPEC-v0.6 §7.2.1, applied here as the `DENY` branch above applies it: a + # presented approval is recorded against the refusal it met, so the history + # connects a live granted approval to what stopped it. It is **not** consumed, + # which is the fast path's point, and a review found this same omission on the + # `DENY` path before it was fixed there. self._refuse_ceiling( action, evaluation, @@ -751,6 +763,7 @@ def execute( attempt=refused_early, ceiling=ceiling, reserved=False, + approval_id=_PRESENTED_APPROVAL.get(None), ) if observation is not None: return self._observed( @@ -1759,13 +1772,16 @@ def _refuse_ceiling( ceiling: int | None, reserved: bool, approval: Approval | None = None, + approval_id: str | None = None, ) -> NoReturn: """Refuse one attempt for the operator's ceiling, and say so in the history (§5.5). Above the ceiling the executor is not called. Where a reservation was taken, the record is released as `FAILED` through `begin_execution` and `fail_effect`, with an error naming the ceiling: `FAILED` is true here, because nothing ran. `EXECUTION_STARTED` is **not** - appended, because that event is the claim that something started. + appended, because that event is the claim that something started. The refused number is + **spent** either way (§5.5): the reservation assigned it, so an operator who later raises + the ceiling buys the difference minus the numbers refusals already consumed. `ActionDenied` and not `DuplicateEffect`, `AmbiguousEffect` or `NotExecuted`: it is the only type in the closed set whose meaning is true, "the action may not run, and `reason` @@ -1773,19 +1789,21 @@ def _refuse_ceiling( is written for exactly that. The receipt is `blocked` rather than `denied` because it describes what stopped the attempt, which is the effect's own history, and it keeps the decision the policy actually reached (`v0.1 §6.1`, `§4.2 A1`). + + **The evidence is written before the release is attempted**, and a refusal of any type is + caught. A review found `InvalidArgument` escaping here: `_checked` raises it where the + record moved under a different `action_id` with a dead lease (`state.py`), and on the old + ordering that left the record `RESERVED` *and* the refusal with no event and no receipt. + The store's own exception still propagates, as `v0.1 §5.5` has it; what changed is that it + can no longer take the evidence with it. + + **On the fast path nothing was reserved**, so `attempt` is the number this call *would* + have been given. Two calls refused in a row therefore carry the same number, which is not + a defect: no number was assigned to either, and the alternative is a receipt that names an + attempt the store never wrote. """ error = f"attempt {attempt} refused: max_attempts is {ceiling} (SPEC-v0.7 §5)" - released: CTRLRunError | None = None - if reserved and effect_key is not None: - try: - self._store.begin_execution(effect_key, action.action_id) - self._store.fail_effect(effect_key, action.action_id, error) - except (DuplicateEffect, AmbiguousEffect) as refused: - # §5.7 — the record moved on while the ceiling was deciding. The refusal - # propagates after the `blocked` receipt, as `v0.1 §5.5` has a store's refusal - # propagate: `AMBIGUOUS` is not something this path may collapse to `FAILED`. - released = refused - presented = approval.approval_id if approval is not None else None + presented = approval.approval_id if approval is not None else approval_id self._append( EventType.EFFECT_RESERVATION_REFUSED, action, @@ -1796,6 +1814,7 @@ def _refuse_ceiling( }, effect_key, approval=approval, + approval_id=approval_id, ) self._record( action, @@ -1804,12 +1823,20 @@ def _refuse_ceiling( started_at, error=error, approval=approval, - approver=self._approver_of(presented), + approval_id=approval_id, + approver=None if approval is not None else self._approver_of(presented), effect_key=effect_key, attempt=attempt, ) - if released is not None: - raise released + if reserved and effect_key is not None: + try: + self._store.begin_execution(effect_key, action.action_id) + self._store.fail_effect(effect_key, action.action_id, error) + except CTRLRunError as refused: + # §5.7 — the record moved on while the ceiling was deciding. The refusal + # propagates, as `v0.1 §5.5` has a store's refusal propagate: `AMBIGUOUS` is not + # something this path may collapse to `FAILED`. + raise refused from None raise ActionDenied( f"{action.name} denied: {error}", reason=BLOCKED_ATTEMPT_CEILING, diff --git a/src/ctrlrun/verify/guarantees.py b/src/ctrlrun/verify/guarantees.py index 6702b8e..9ede1b6 100644 --- a/src/ctrlrun/verify/guarantees.py +++ b/src/ctrlrun/verify/guarantees.py @@ -64,7 +64,9 @@ class Guarantee: ), Guarantee( "G15", - "a renewal past the operator's ceiling is refused", + # Exactly `report._TITLE_WIDTH`. A longer title is the one thing that breaks the CLI + # table's alignment, and "a renewal past the operator's ceiling is refused" was 48. + "renewal past the ceiling refused", ( "v0.1 §5.4", "v0.7 §8 T240", @@ -160,8 +162,16 @@ class Guarantee: "no action verify can drive to allow or approve declares both `effect:` and `max_attempts`" ) CEILING_BOUND: Final = 100 + +#: **Scoped to what verify can select**, on `NO_CEILING_DECLARED`'s shape and for its reason. An +#: earlier wording, "every declared max_attempts is above verify's bound", was false of a document +#: whose deny-only action declares `max_attempts: 3`: the fallback selection re-applies the effect +#: and ceiling filters and drops only the bound, so the sentence can only ever describe the actions +#: verify can drive. That is the identical defect §8.9 caught in the sibling sentence, and an N/A +#: reason that is not true of the operator's document is a false green (§8.9's opening MUST). CEILING_ABOVE_BOUND: Final = ( - f"every declared max_attempts is above verify's bound of {CEILING_BOUND} attempts" + "every action verify can drive to allow or approve that declares both `effect:` and " + f"`max_attempts` declares one above verify's bound of {CEILING_BOUND} attempts" ) #: SPEC-v0.7 §8.9 — the reason G5 (and G14, when item 3 lands it) reports where the operator's diff --git a/src/ctrlrun/verify/scenarios.py b/src/ctrlrun/verify/scenarios.py index 2e406a4..fc9a66d 100644 --- a/src/ctrlrun/verify/scenarios.py +++ b/src/ctrlrun/verify/scenarios.py @@ -2357,6 +2357,11 @@ def g15(self) -> GuaranteeResult: needs_effect=True, needs_ceiling=True, ceiling_bound=reg.CEILING_BOUND ) if selection is None: + # The fallback drops the bound and **keeps** every other filter, so what it finds is + # an action verify could otherwise drive. `CEILING_ABOVE_BOUND` is worded to say + # exactly that and no more: a deny-only action's low ceiling is not something this + # selection ever looked at, and a sentence claiming "every declared" would be false + # of the operator's document (§8.9's opening MUST). if self.select(needs_effect=True, needs_ceiling=True) is not None: return self.na("G15", reg.CEILING_ABOVE_BOUND) return self.na("G15", self.unselected(reg.NO_CEILING_DECLARED)) diff --git a/tests/test_attempt_cap.py b/tests/test_attempt_cap.py index 7315ebb..b474268 100644 --- a/tests/test_attempt_cap.py +++ b/tests/test_attempt_cap.py @@ -38,6 +38,7 @@ ApprovalRequired, Control, InMemoryStateStore, + InvalidArgument, NotExecuted, Policy, PolicyError, @@ -149,27 +150,57 @@ def _not_executed() -> NotExecuted: return NotExecuted("the remote rejected it before doing anything") -@pytest.fixture(params=["in-memory", "sqlite"]) +@pytest.fixture( + params=[ + "in-memory", + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + not os.environ.get("CTRLRUN_TEST_POSTGRES"), + reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against", + ), + ), + ] +) def stores(request, tmp_path, fake_clock): """A factory for stores on whichever backend, so one test can build two (T247). `conftest.state_store` hands out one store, and the positive control needs a second: the same sequence under a document with no ceiling, compared record for record. + + **Postgres is one of the three**, which §8.4's T247 asks for in its first sentence: without + it the ceiling's only Postgres coverage is T247's property assertion, which catches a deleted + check about a quarter of the time and a deleted fast path never. Each store gets a scratch + schema of its own, dropped afterwards (`v0.6 §4.1`), so nothing is left in `public`. """ made: list[object] = [] + schemas: list[str] = [] def make(): - store = ( - InMemoryStateStore(clock=fake_clock) - if request.param == "in-memory" - else SQLiteStateStore(tmp_path / f"state-{len(made)}.db", clock=fake_clock) - ) + if request.param == "in-memory": + store = InMemoryStateStore(clock=fake_clock) + elif request.param == "sqlite": + store = SQLiteStateStore(tmp_path / f"state-{len(made)}.db", clock=fake_clock) + else: + from ctrlrun.postgres import PostgresStateStore + + url = os.environ["CTRLRUN_TEST_POSTGRES"] + name = f"cap_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(url, name) + schemas.append(name) + store = PostgresStateStore(url, schema=name, clock=fake_clock) made.append(store) return store yield make for store in made: store.close() + if schemas: + from ctrlrun.postgres import PostgresStateStore + + for name in schemas: + PostgresStateStore.drop_schema(os.environ["CTRLRUN_TEST_POSTGRES"], name) def _control(document, store, fake_clock): @@ -274,6 +305,17 @@ def _stale_read(store, interleave): return _window(store, "get_effect", interleave) +@contextmanager +def monkeypatched(owner, name, replacement): + """One attribute swapped for the length of a `with`, restored on the way out.""" + original = getattr(owner, name) + setattr(owner, name, replacement) + try: + yield + finally: + setattr(owner, name, original) + + def _renew_and_fail(control, remote, payment_id="txn_1"): """One whole attempt by somebody else: renew the key, dispatch, and fail.""" with pytest.raises(NotExecuted): @@ -343,8 +385,19 @@ def test_T241_the_check_after_the_reservation_never_calls_the_executor(stores, f def _projection(store): - """What a run left behind, in the terms 0.6.1 and 0.7 must agree on.""" - events = [(str(event.type), dict(event.data), event.effect_key) for event in store.events()] + """What a run left behind, in the terms 0.6.1 and 0.7 must agree on. + + `CLOCK_SKEW_DETECTED` is dropped, and only that: on Postgres every scratch store is opened + with this suite's frozen clock, so each one measures a real skew of several days and reports + it with its own microseconds (SPEC-v0.7 §1.4 item 6, §3.8). That is item 1's event and item + 1's tests grade it; what this comparison is about is whether a ceiling under which nothing + is refused changes anything the ceiling owns. + """ + events = [ + (str(event.type), dict(event.data), event.effect_key) + for event in store.events() + if str(event.type) != "CLOCK_SKEW_DETECTED" + ] receipts = [ (receipt.result, receipt.attempt, receipt.effect_key) for receipt in _receipts(store) ] @@ -656,6 +709,15 @@ def test_T245b_the_fast_path_leaves_a_presented_approval_granted(stores, fake_cl assert store.get_approval(second).status == "granted" assert _refusals(store)[-1].data["reason"] == CEILING + # SPEC-v0.6 §7.2.1, as the `DENY` path applies it: the refusal is **recorded against** the + # approval it met, so the history connects a live granted approval to what stopped it. A + # review found `approval_id=None` here while an unspent approval sat in the store, which is + # the same omission that review found on the `DENY` path. + assert _refusals(store)[-1].approval_id == second + blocked = _receipts(store)[-1] + assert blocked.approval_id == second + assert blocked.approver == "ops@example.com" + def test_T245b_the_fast_path_reserves_nothing(stores, fake_clock): store = stores() @@ -709,7 +771,7 @@ def test_T245b_without_the_fast_path_the_three_do_not_fail_alike(stores, fake_cl BOUND = 180.0 CHILD = textwrap.dedent(""" - import json, os, sys + import json, os, sys, time job = json.loads(sys.stdin.read()) sys.path.insert(0, job["src"]) import ctrlrun @@ -730,6 +792,7 @@ def refund(payment_id, amount): raise NotExecuted("the remote rejected it before doing anything") refused = None + started = time.time() for _ in range(job["rounds"]): try: with context(agent="refund-agent"): @@ -741,6 +804,7 @@ def refund(payment_id, amount): break except CTRLRunError: continue + finished = time.time() record = store.get_effect("refund:" + job["payment_id"]) store.close() print(json.dumps({ @@ -748,10 +812,29 @@ def refund(payment_id, amount): "refused": refused, "attempt": None if record is None else record.attempt, "state": None if record is None else str(record.state), + "started": started, + "finished": finished, })) """) +def _overlapping(results): + """The largest number of children whose `[started, finished]` windows all overlap. + + The signal a serialised run cannot produce. Each child reports the wall clock it took either + side of its own loop, so two children overlap when neither finished before the other began. + """ + most = 0 + for probe in results: + together = [ + other + for other in results + if other["started"] <= probe["finished"] and probe["started"] <= other["finished"] + ] + most = max(most, len(together)) + return most + + @postgres def test_T247_no_more_than_N_dispatches_across_separate_processes(): """`v0.6`'s multi-process standard: separate OS processes, one Postgres, one key. @@ -822,11 +905,65 @@ def test_T247_no_more_than_N_dispatches_across_separate_processes(): assert all(result["refused"] in (None, CEILING) for result in results), ( f"a process was refused for a reason this run cannot explain: {results}" ) + # **And the run has to have contended**, asserted on something only contention produces. + # Every assertion above is satisfied by six processes running one after another, which is + # what this test looked like before a review replaced the feed-all-then-wait loop with a + # `wait()` per child and watched it pass 3 out of 3. A serialised run has no two windows + # that overlap; a contended one has several. + assert _overlapping(results) >= 2, ( + f"no two children ran at the same time, so nothing was contended: {results}" + ) # --- T248: what happens to an approval on a refused attempt ----------------------------- +def test_T248_the_reconcile_route_asks_a_human_for_an_attempt_that_can_never_run( + stores, fake_clock +): + """The cost §5.5 states rather than closes, pinned so the claim cannot drift back. + + The fast path refuses only a `FAILED` record, normatively, so an `AMBIGUOUS` one goes + through to `_secure` -- and `_secure` reaches the approval gate **before** any reconcile. + So on §5.5's own public route, which is the one T245 and G15 are built on, a human is asked + for an attempt that the check will then refuse, and the yes they give is consumed by the + reservation that gets refused. + + This is not the adapter's `Control.evaluate` exception: it is the kernel's primary route. + §5.5 states the cost and does not close it, because re-reading the record between the + reconcile and the second take would destroy the seamless route the check's own test needs. + """ + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + remote = Remote(TimeoutError("the response was lost"), "never reached") + with pytest.raises(TimeoutError): + _call(control, remote, approval=_grant(control, store)) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + requested_before = len(_events(store, EventType.APPROVAL_REQUESTED)) + + # Attempt 2 presents nothing and carries the hook. A NEW request is created and the caller + # is told to go and find a human, although attempt 2 can never run under a ceiling of 1. + with pytest.raises(ApprovalRequired) as asked: + _call(control, remote, reconcile=lambda key: "not_executed") + assert len(_events(store, EventType.APPROVAL_REQUESTED)) == requested_before + 1, ( + "the approval gate ran before the reconcile, so a human was asked" + ) + request_id = asked.value.request_id + assert store.get_approval(request_id).status == "pending" + + # The human says yes, the agent retries, and the yes is spent on the refusal. + store.grant_approval(request_id, "ops@example.com") + with pytest.raises(ActionDenied) as refused: + _call(control, remote, approval=request_id, reconcile=lambda key: "not_executed") + assert refused.value.reason == CEILING + assert store.get_approval(request_id).status == "consumed", ( + "the reservation consumed it in the transaction the check then refused" + ) + assert remote.calls == 1, "one dispatch under a ceiling of 1, and one wasted answer" + assert _receipts(store)[-1].approval_id == request_id + + def test_T248_the_check_leaves_the_approval_consumed_and_the_receipt_names_it(stores, fake_clock): store = stores() control = _control(APPROVE_CEILING_2, store, fake_clock) @@ -936,6 +1073,100 @@ def lease_lapses_and_somebody_ambiguates(): assert store.get_effect(KEY).state is EffectState.AMBIGUOUS +def test_T249_a_store_refusal_of_any_type_still_leaves_the_evidence(stores, fake_clock): + """The refusal takes nothing with it (§5.5, `v0.1 §5.5`). + + `_checked` raises `InvalidArgument`, not `DuplicateEffect` or `AmbiguousEffect`, where the + record moved under a different `action_id` with a dead lease. A review found that escaping + before the event and the receipt were written, leaving the record `RESERVED` and the refusal + with no evidence at all. The evidence is written first now, and every `CTRLRunError` the + release raises still propagates. + """ + store = stores() + control = _control(ALLOW_CEILING_3, store, fake_clock) + remote = Remote(_not_executed(), _not_executed(), TimeoutError("the response was lost")) + for _ in range(2): + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(TimeoutError): + _call(control, remote) + + def refusing(self, *args, **kwargs): + raise InvalidArgument("the record moved under another action with a dead lease") + + with monkeypatched(type(store), "begin_execution", refusing), pytest.raises(InvalidArgument): + _call(control, remote, reconcile=lambda key: "not_executed") + + assert _refusals(store)[-1].data["reason"] == CEILING + blocked = _receipts(store)[-1] + assert blocked.result is ReceiptResult.BLOCKED + assert "max_attempts is 3" in (blocked.error or "") + assert remote.calls == 3 + + +# --- what the refused attempt number costs, and what the ceiling does not bound ---------- + + +def test_a_refused_attempt_number_is_spent(stores, fake_clock): + """§5.5: the reservation assigned it, so raising the ceiling buys less than the difference. + + §5.7 justifies counting an attempt a human resolved because *"it was dispatched"*. The number + a check-path refusal burns was not dispatched, and it is spent all the same, because the + store assigned it before the check could look. An operator raising `max_attempts` from 2 to 4 + buys one more dispatch, not two, and the spec says so rather than leaving it to be found. + """ + store = stores() + control = _control(ALLOW_CEILING_2, store, fake_clock) + remote = Remote(_not_executed(), TimeoutError("the response was lost"), "ok", "ok") + with pytest.raises(NotExecuted): + _call(control, remote) + with pytest.raises(TimeoutError): + _call(control, remote) + with pytest.raises(ActionDenied) as refused: + _call(control, remote, reconcile=lambda key: "not_executed") + assert refused.value.reason == CEILING + assert remote.calls == 2 + + record = store.get_effect(KEY) + assert record.state is EffectState.FAILED + assert record.attempt == 3, "the refused number was assigned, and it is gone" + + raised = _control( + ALLOW_CEILING_3.replace("max_attempts: 3", "max_attempts: 4"), store, fake_clock + ) + assert _call(raised, remote) == "re_txn_1-3" + assert remote.calls == 3, "raising 2 to 4 bought one more dispatch, not two" + + +def test_the_ceiling_bounds_attempts_and_not_executor_invocations(stores, fake_clock): + """§5.7: `Control.resume` reserves nothing, and the consequence is stated. + + One attempt can invoke the executor many times: a `Suspended` executor holds attempt 1 and + each `resume` runs it again on that same attempt. The gateway bounds this with + `max_elicitation_rounds` (`v0.2 §6.9.2`); a direct `Control.resume` caller has no bound, and + `max_attempts` is not one. It bounds attempts, which is what a provider dispatch costs. + """ + store = stores() + control = _control(ALLOW_CEILING_1, store, fake_clock) + rounds = {"n": 0} + + @protect("stripe.refund", effect="refund:{payment_id}", control=control) + def refund(payment_id: str, amount: int) -> str: + rounds["n"] += 1 + raise Suspended(f"round-{rounds['n']}") + + with context(agent="refund-agent"), pytest.raises(Suspended): + refund(payment_id="txn_1", amount=200) + for expected in range(2, 6): + with pytest.raises(Suspended): + control.resume(f"round-{expected - 1}", lambda: refund.__wrapped__("txn_1", 200)) + assert rounds["n"] == expected + + record = store.get_effect(KEY) + assert record.attempt == 1, "every round was the same attempt" + assert rounds["n"] == 5, "five executor invocations under max_attempts: 1" + + # --- T250: observe mode, resume, and resolved attempts ---------------------------------- @@ -1137,7 +1368,10 @@ def test_a_committed_effect_is_still_a_duplicate_and_not_a_ceiling_refusal(store G15_NOT_DECLARED = ( "no action verify can drive to allow or approve declares both `effect:` and `max_attempts`" ) -G15_ABOVE_BOUND = "every declared max_attempts is above verify's bound of 100 attempts" +G15_ABOVE_BOUND = ( + "every action verify can drive to allow or approve that declares both `effect:` and " + "`max_attempts` declares one above verify's bound of 100 attempts" +) CEILING_FORBIDS_RENEWAL = ( "every action with an `effect:` template that verify can select (a decision of allow or " "approve under a grant that covers it) declares max_attempts: 1, so no renewal can happen" @@ -1255,6 +1489,48 @@ def test_T252_G15_is_not_applicable_above_verifys_bound(tmp_path): assert result.reason == G15_ABOVE_BOUND +def test_T252_the_above_bound_reason_is_true_of_a_document_with_a_low_deny_only_ceiling(tmp_path): + """§8.9's opening MUST: every N/A reason is a sentence true of the operator's document. + + The fallback selection re-applies the effect and ceiling filters and drops only the bound, so + it can only ever describe the actions verify can **drive**. An earlier wording said "every + declared max_attempts is above verify's bound" and was false here: `acme.beta` declares 3. + That is the identical defect §8.9 already caught and fixed in the sibling sentence. + """ + from ctrlrun.verify import Status + + document = """ +schema: ctrlrun.policy/v5 +actions: + acme.alpha: + effect: "alpha:{ticket}" + max_attempts: 1000 + rules: + - when: { amount_gte: 0, amount_lte: 1000 } + decision: allow + - decision: deny + acme.beta: + effect: "beta:{ticket}" + max_attempts: 3 + decision: deny +""" + result = _verify(tmp_path, document, only=("G15",))["G15"] + assert result.status is Status.NOT_APPLICABLE + assert result.reason == G15_ABOVE_BOUND + assert "every declared max_attempts" not in result.reason, ( + "the sentence must not claim anything about a ceiling verify never looked at" + ) + + +def test_T252_the_catalogue_titles_fit_the_report_table(tmp_path): + """A title wider than `report._TITLE_WIDTH` breaks the CLI table's alignment.""" + from ctrlrun.verify import guarantees as reg + from ctrlrun.verify.report import _TITLE_WIDTH + + too_wide = {g.id: len(g.title) for g in reg.GUARANTEES if len(g.title) > _TITLE_WIDTH} + assert not too_wide, too_wide + + def test_T252_a_kernel_with_the_check_deleted_fails_G15(tmp_path, monkeypatch): """The control §8.9 asks for: G15 must be able to fail, and on the check's own mechanism. From 401bc2b00bd87f98d8ce43b60c86e004d95ca541 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 01:29:09 +0530 Subject: [PATCH 3/3] Answer round two: a guard that graded nothing, and three sections that were wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing blocking, and three of the seven are this specification being wrong about its own code. The try/except round one wrapped the release in is an EQUIVALENT MUTANT: the reviewer deleted it whole and got 114 of 114 green, because it caught only to re-raise the same object and nothing observes __suppress_context__. Deleted. A store refusal propagates by not being caught, which is what §5.5's prose said all along. The mutation table loses the row that claimed to grade it: a red row for an equivalent mutant is a false green, and that one survived a review round by looking like the fix for a real finding. What is load-bearing is that the evidence is written FIRST, which T249 grades and M26 kills. §5.5's numbered list put the release before the event and the receipt; the code does the reverse, and the release may not happen at all. Renumbered to the code's order, step 4 qualified "where the release succeeds", and §5.2 now says a failed release lands exactly where a crash does: RESERVED under a live lease, AMBIGUOUS when it lapses, a human resolving an effect that did not run. And the merge put item 5's precondition provider in front of the ceiling with no section saying so. Measured: on the reconcile route under max_attempts: 1 a doomed attempt calls the operator's provider three times before the check refuses, one on the request pass and two on the retry, because _recheck runs once per _take and that route takes twice. A provider that raises there makes the refusal precondition_unavailable rather than attempt_ceiling, writes no effect record and never runs the reconcile hook, so the operator is told the wrong reason for an attempt that could never run. The sequential route calls it zero times. All three numbers are in §5.5 and pinned by three tests. Four smaller: T248 counts its reconcile hook instead of inferring that the approval gate ran first; _projection normalises CLOCK_SKEW_DETECTED's volatile fields instead of dropping the event, so the count still counts; the catalogue ordering test keeps its place for locality and BY_ID's order, and its docstring no longer claims to be the only guard, which 2013940's commit message also claimed and which is untrue: tests/test_verify.py has asserted the same ordering since before this branch; and the changelog's max_attempts bullet wraps. --- CHANGELOG.md | 10 +- docs/SPEC-v0.7.md | 103 +++++++++++++++---- src/ctrlrun/control.py | 18 ++-- tests/test_attempt_cap.py | 202 +++++++++++++++++++++++++++++++++++--- 4 files changed, 288 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82b5de1..31a4253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,11 +30,11 @@ any change to one appears here. `ctrlrun.guarantees/v3`; the store conformance suite gains a `clock` case, `not_applicable` on SQLite and the in-memory store because neither has a clock of its own. - **The attempt ceiling, `max_attempts`** (SPEC-v0.7 §5, item 4, and the amendment to - `docs/SPEC-v0.1.md` §5.4). A new action-entry policy key, an integer of at least 1, bounding the - **attempts** that may execute on one effect key, the first included: `max_attempts: 3` is the - first attempt and two renewals. It needs `schema: ctrlrun.policy/v5`, a new schema version that - is a superset of `v4` as `v4` is of `v3`; `0`, a negative, a `bool`, a float, a string and a - mapping are each a `PolicyError` at load, naming the key, the action and the line. The ceiling + `docs/SPEC-v0.1.md` §5.4). A new action-entry policy key, an integer of at least 1, bounding + the **attempts** that may execute on one effect key, the first included: `max_attempts: 3` is + the first attempt and two renewals. It needs `schema: ctrlrun.policy/v5`, a new schema version + that is a superset of `v4` as `v4` is of `v3`; `0`, a negative, a `bool`, a float, a string and + a mapping are each a `PolicyError` at load, naming the key, the action and the line. The ceiling is inside the policy hash, so a receipt records which one refused an attempt. **An attempt, not an executor invocation**: a `Suspended` executor holds its reservation and every `Control.resume` runs on that same attempt, so an elicitation loop is one dispatch however diff --git a/docs/SPEC-v0.7.md b/docs/SPEC-v0.7.md index 1231ca2..a050c65 100644 --- a/docs/SPEC-v0.7.md +++ b/docs/SPEC-v0.7.md @@ -923,12 +923,15 @@ answer, for an attempt that could never run. §5.5 states both, and neither is c what it costs: every later attempt on that key is also over the ceiling, so no approval spent here opens anything until the operator raises `max_attempts`, and nothing ever executes. -**What a crash between the reservation and the release leaves.** The record is `RESERVED`, or -`EXECUTING` if the crash fell after `begin_execution`, under a live lease. When the lease lapses the -next reservation attempt declares it `AMBIGUOUS` (`v0.1 §5.3 E3`), and a human or a `reconcile` hook -must resolve it **although nothing ran**. That is exactly what a crash between the reservation and the -executor call leaves today, and it costs a human, never an execution. The fast path makes it rare, -since only a race reaches the check after the reservation. +**What a crash between the reservation and the release leaves, and what a failed release leaves with +it.** The record is `RESERVED`, or `EXECUTING` if the crash fell after `begin_execution`, under a live +lease. **A release the store refuses lands in exactly the same place** (§5.5 step 4): the record stays +`RESERVED` at the refused attempt under its lease, and nothing distinguishes it from a process that +stopped there. Either way, when the lease lapses the next reservation attempt declares it `AMBIGUOUS` +(`v0.1 §5.3 E3`), and a human or a `reconcile` hook must resolve it **although nothing ran**. That is +exactly what a crash between the reservation and the executor call leaves today, and it costs a human, +never an execution. The fast path makes it rare, since only a race reaches the check after the +reservation. ### 5.3 The key @@ -1002,25 +1005,30 @@ and before `begin_execution`** (`control.py:712-719`). That is the check, and it where the attempt number is unique per key (§5.6), at most `max_attempts` reservations can ever carry a number within the ceiling, whatever the concurrency. -Above the ceiling: +Above the ceiling, **in the order the code does them**, because the order is the argument: 1. **The executor is not called.** -2. **The record is released as `FAILED`**, through `begin_execution` then `fail_effect`, with an - `error` naming the ceiling: `attempt 4 refused: max_attempts is 3 (SPEC-v0.7 §5)`. `FAILED` is true: - nothing ran. `begin_execution` here is a state transition that `fail_effect` requires, and - `EXECUTION_STARTED` is **not** appended, because that event is the claim that something started. -3. **`EFFECT_RESERVATION_REFUSED` is appended** with `data.reason = "attempt_ceiling"`, +2. **`EFFECT_RESERVATION_REFUSED` is appended** with `data.reason = "attempt_ceiling"`, `data.attempt` and `data.max_attempts`, on the existing event type, so the history says why. -4. **A `blocked` receipt** is written, keeping the decision the policy reached (`v0.1 §4.2 A1`'s +3. **A `blocked` receipt** is written, keeping the decision the policy reached (`v0.1 §4.2 A1`'s precedent) and carrying the approval where one was consumed or presented. -5. **`ActionDenied(reason="attempt_ceiling")` is raised.** - -**The evidence is written before the release is attempted**, and a store refusal of any type is caught -and re-raised after it. `begin_execution` and `fail_effect` can refuse with `DuplicateEffect` or -`AmbiguousEffect` where the record moved (§5.7), and with `InvalidArgument` where it moved under a -different `action_id` with a dead lease. On any of those the caller gets the store's exception, as -`v0.1 §5.5` requires, and the event and the receipt exist either way: a refusal that took its own -evidence with it would leave a `RESERVED` record nothing in the history explains. +4. **The record is released as `FAILED`, where the release succeeds**, through `begin_execution` + then `fail_effect`, with an `error` naming the ceiling: + `attempt 4 refused: max_attempts is 3 (SPEC-v0.7 §5)`. `FAILED` is true: nothing ran. + `begin_execution` here is a state transition that `fail_effect` requires, and + `EXECUTION_STARTED` is **not** appended, because that event is the claim that something started. +5. **`ActionDenied(reason="attempt_ceiling")` is raised**, unless step 4 refused, in which case the + store's own exception propagates instead. + +**Steps 2 and 3 come before step 4, and that is the whole of why the order is written down.** +`begin_execution` and `fail_effect` can refuse: with `DuplicateEffect` or `AmbiguousEffect` where the +record moved (§5.7), and with `InvalidArgument` where it moved under a different `action_id` with a +dead lease. The caller then gets the store's exception, as `v0.1 §5.5` requires, and the event and the +receipt exist either way, because they were written first: a refusal that took its own evidence with it +would leave a `RESERVED` record nothing in the history explains. **Step 4 may therefore not happen at +all**, and the record is then `RESERVED` at the refused attempt under a live lease while the receipt +says `blocked` at that same attempt. What happens next is what §5.2's last paragraph describes, and it +costs a human rather than an execution. **The refused number is spent.** The store assigned attempt *n* + 1 before the check could look at it, and releasing the record `FAILED` does not give it back: the next renewal is *n* + 2. So an operator @@ -1078,6 +1086,20 @@ reader should assume there are others rather than read this as a closed set: reachable through a public route, with no seam" below, leaving the check with no seamless route and G15 with nothing to grade it through. The cost is one wasted answer; the alternative is an unexercised guarantee, and `v0.4 §1.3`'s rule is that a guarantee that could not have failed is not a pass. + **And §6's precondition provider runs in front of the ceiling on this route, measured.** `_recheck` + sits immediately before each `_take` (§6.2), and this route takes twice, so under `max_attempts: 1` a + doomed attempt calls the operator's provider **three times** before it is refused: once on the request + pass that creates the approval, and twice on the retry. §6.6 says the provider is spent only where its + answer can matter, and here its answer cannot: the ceiling refuses whatever it returns. **Worse, a + provider that raises on that attempt changes the reason the operator is told**: the refusal is + `ApprovalMismatch(reason="precondition_unavailable")` and not `attempt_ceiling`, no effect record is + written, the record stays `AMBIGUOUS` at N, and the `reconcile` hook never runs. Both are consequences + of the ordering `v0.3 §4.3.1` fixes and §5.5 amends, not of either mechanism alone, and both are + stated here rather than closed: moving the ceiling in front of the fetch would mean resolving the + record before the approval gate on a route whose whole point is that it does not, which is the same + seam this bullet declines above. **The sequential route is clean**: where the record is already + `FAILED` at the ceiling the fast path refuses before the approval gate and the provider is called + **zero** times, which is §6.6's principle holding wherever the fast path can answer. - **`Control.evaluate` does not see the ceiling** (§7). An adapter decides whether to interrupt for a human through `ctrlrun.adapter.needs_approval`, which calls it (`adapter.py:408-450`); it takes an `Action`, not an effect key, and it writes nothing, so it cannot resolve a record to count on. A framework can therefore @@ -3207,6 +3229,45 @@ and `Control.evaluate`'s docstring now says it does not see the ceiling. G15's t `report._TITLE_WIDTH`'s 32, the only one over, so it is "renewal past the ceiling refused" and a test asserts no title ever exceeds the width again. +**Round two found nothing blocking, and its seven notes are mostly about this document being wrong about the +code.** Three were. + +*A guard that was an equivalent mutant, reported as a red row.* Round one's fix for the escaping +`InvalidArgument` did two things: it moved the evidence above the release, and it wrapped the release in +`try/except CTRLRunError: raise refused from None`. Only the first is load-bearing. The reviewer deleted the +whole `try`/`except` and got 114 of 114 green, because it caught only to re-raise the same object and nothing +observes `__suppress_context__` (which is `None` there in any case). The clause is **deleted**: a store refusal +now propagates by not being caught, which is what §5.5's prose said all along, and the mutation table loses the +row that claimed to grade it. **A red row for an equivalent mutant is a false green in the table**, which is +worse than no row, and this one survived a round of review by looking like the fix for a real finding. + +*§5.5's numbered list was in the wrong order and too certain.* It put the release second, before the event and +the receipt; the code does the event, the receipt, then the release, which is the fix round one made and the list +did not follow. And the release **may not happen at all**: measured, after an `InvalidArgument` release the +record sits `RESERVED` at the refused attempt under a live lease while the receipt says `blocked` at that same +attempt. The list is renumbered to the code's order, step 4 is qualified "where the release succeeds", and +§5.2's crash paragraph now says that a failed release lands in the same place a crash does, because it does. + +*§6's provider runs in front of the ceiling, and no section said so.* The merge put item 5's precondition +recheck immediately before each `_take`, and the reconcile route takes twice, so under `max_attempts: 1` a +doomed attempt calls the operator's provider **three times** before the check refuses: once on the request pass +and twice on the retry. Worse, a provider that raises on that attempt makes the refusal +`ApprovalMismatch(reason="precondition_unavailable")` rather than `attempt_ceiling`, writes no effect record, and +never runs the `reconcile` hook, so the operator is told the wrong reason for an attempt that could never run. +Both are measured, both are in §5.5's reconcile bullet, and both are stated rather than closed for the same +reason the human's answer is: moving the ceiling in front of the fetch needs the seam that bullet already +declines. The sequential route calls the provider **zero** times, which is §6.6's principle holding wherever the +fast path can answer, and that is said too. Three tests pin all three numbers. + +The other four were smaller and all four were true. T247's overlap assertion is real, but the docstring claiming +"nothing but this would notice" the catalogue swap was not: `tests/test_verify.py`'s catalogue test has asserted +the same ordering since before this branch, and the reviewer showed both tests failing on the swap. The test +stays for locality and for `BY_ID`'s own order, which the other one does not assert, and the docstring says so. +T248's "the approval gate ran before the reconcile" was **inferred**: the hook answered the same whenever it ran, +so the assertion held either way. It counts now, and asserts zero calls at the moment the human is asked. +`_projection` **normalises** `CLOCK_SKEW_DETECTED`'s three volatile fields rather than dropping the event, so the +comparison still counts them and still fixes where they fall. + ### 12.5 Item 5: precondition fingerprints It narrows; the residual window of §6.7 is T261b's, and nothing written for this item says otherwise. diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index ccd25e2..09adf3d 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -2126,14 +2126,16 @@ def _refuse_ceiling( attempt=attempt, ) if reserved and effect_key is not None: - try: - self._store.begin_execution(effect_key, action.action_id) - self._store.fail_effect(effect_key, action.action_id, error) - except CTRLRunError as refused: - # §5.7 — the record moved on while the ceiling was deciding. The refusal - # propagates, as `v0.1 §5.5` has a store's refusal propagate: `AMBIGUOUS` is not - # something this path may collapse to `FAILED`. - raise refused from None + # §5.7 — where the record moved on while the ceiling was deciding, one of these + # refuses and **that refusal propagates**, in place of the `ActionDenied` below, as + # `v0.1 §5.5` has a store's refusal propagate: `AMBIGUOUS` is not something this + # path may collapse to `FAILED`. It propagates by not being caught. A round of + # review found the `try`/`except CTRLRunError: raise` that used to stand here to be + # an equivalent mutant, green with the whole clause deleted, because it caught only + # to re-raise the same object. What is load-bearing is that the evidence above is + # written **first**, which T249 grades and which a reordering mutant kills. + self._store.begin_execution(effect_key, action.action_id) + self._store.fail_effect(effect_key, action.action_id, error) raise ActionDenied( f"{action.name} denied: {error}", reason=BLOCKED_ATTEMPT_CEILING, diff --git a/tests/test_attempt_cap.py b/tests/test_attempt_cap.py index acc7068..f416931 100644 --- a/tests/test_attempt_cap.py +++ b/tests/test_attempt_cap.py @@ -35,6 +35,7 @@ from ctrlrun import ( ActionDenied, AmbiguousEffect, + ApprovalMismatch, ApprovalRequired, Control, InMemoryStateStore, @@ -146,6 +147,24 @@ def __call__(self) -> str: return f"re_txn_1-{self.calls}" +class _Hook: + """A `reconcile` hook that counts its calls, so "before" and "after" are observed. + + `reconcile=lambda key: "not_executed"` answers the same whenever it runs, so a test that + asserts only what the hook's answer produced cannot tell the order it ran in. + """ + + def __init__(self, answer: str = "not_executed") -> None: + self.calls = 0 + self.keys: list[str] = [] + self._answer = answer + + def __call__(self, effect_key: str) -> str: + self.calls += 1 + self.keys.append(effect_key) + return self._answer + + def _not_executed() -> NotExecuted: return NotExecuted("the remote rejected it before doing anything") @@ -384,19 +403,33 @@ def test_T241_the_check_after_the_reservation_never_calls_the_executor(stores, f # --- T242: the positive control -- under the ceiling, 0.6.1's behaviour ----------------- +#: The three fields of `CLOCK_SKEW_DETECTED` that differ between two measurements of the same +#: thing: the microseconds measured, the instant, and the round trip's half (SPEC-v0.7 §3.4). +_VOLATILE_SKEW_FIELDS = ("skew_us", "measured_at", "bound_us") + + def _projection(store): """What a run left behind, in the terms 0.6.1 and 0.7 must agree on. - `CLOCK_SKEW_DETECTED` is dropped, and only that: on Postgres every scratch store is opened - with this suite's frozen clock, so each one measures a real skew of several days and reports - it with its own microseconds (SPEC-v0.7 §1.4 item 6, §3.8). That is item 1's event and item - 1's tests grade it; what this comparison is about is whether a ceiling under which nothing - is refused changes anything the ceiling owns. + `CLOCK_SKEW_DETECTED` is **normalised, not dropped**: on Postgres every scratch store is + opened with this suite's frozen clock, so each one measures a real skew of several days and + reports it with its own microseconds (SPEC-v0.7 §1.4 item 6, §3.8). Replacing those three + fields keeps the event in the sequence, so the comparison still counts them and still fixes + where they fall: dropping the event would let a capped run that opened one more store than + the uncapped one compare equal. Nothing reaches that today, and the stronger form is free. """ events = [ - (str(event.type), dict(event.data), event.effect_key) + ( + str(event.type), + { + key: ("" if key in _VOLATILE_SKEW_FIELDS else value) + for key, value in event.data.items() + } + if str(event.type) == "CLOCK_SKEW_DETECTED" + else dict(event.data), + event.effect_key, + ) for event in store.events() - if str(event.type) != "CLOCK_SKEW_DETECTED" ] receipts = [ (receipt.result, receipt.attempt, receipt.effect_key) for receipt in _receipts(store) @@ -942,10 +975,17 @@ def test_T248_the_reconcile_route_asks_a_human_for_an_attempt_that_can_never_run requested_before = len(_events(store, EventType.APPROVAL_REQUESTED)) + # **Counted, not inferred** (mutation pattern 4). "The approval gate ran before the + # reconcile" is the whole finding, and a request created *after* the hook had run would + # satisfy the count above just as well. The hook counts its own calls, and the assertion is + # that it had not been called when the human was asked. + hook = _Hook("not_executed") + # Attempt 2 presents nothing and carries the hook. A NEW request is created and the caller # is told to go and find a human, although attempt 2 can never run under a ceiling of 1. with pytest.raises(ApprovalRequired) as asked: - _call(control, remote, reconcile=lambda key: "not_executed") + _call(control, remote, reconcile=hook) + assert hook.calls == 0, "the reconcile hook ran first, so this is not the route §5.5 names" assert len(_events(store, EventType.APPROVAL_REQUESTED)) == requested_before + 1, ( "the approval gate ran before the reconcile, so a human was asked" ) @@ -955,8 +995,9 @@ def test_T248_the_reconcile_route_asks_a_human_for_an_attempt_that_can_never_run # The human says yes, the agent retries, and the yes is spent on the refusal. store.grant_approval(request_id, "ops@example.com") with pytest.raises(ActionDenied) as refused: - _call(control, remote, approval=request_id, reconcile=lambda key: "not_executed") + _call(control, remote, approval=request_id, reconcile=hook) assert refused.value.reason == CEILING + assert hook.calls == 1, "the retry is where the hook runs, once, after the approval gate" assert store.get_approval(request_id).status == "consumed", ( "the reservation consumed it in the transaction the check then refused" ) @@ -1104,6 +1145,137 @@ def refusing(self, *args, **kwargs): assert remote.calls == 3 +# --- §6's provider in front of the ceiling, on the route §5.5 states --------------------- + + +def _with_provider(control, remote, provider, *, approval=None, reconcile=None): + """One attempt whose `@protect` names a precondition provider (SPEC-v0.7 §6.2).""" + extra = {"reconcile": reconcile} if reconcile is not None else {} + + @protect( + "stripe.refund", + effect="refund:{payment_id}", + control=control, + preconditions=provider, + **extra, + ) + def refund(payment_id: str, amount: int) -> str: + return remote() + + with context(agent="refund-agent"): + if approval is None: + return refund(payment_id="txn_1", amount=200) + with with_approval(approval): + return refund(payment_id="txn_1", amount=200) + + +class _Provider: + """A precondition provider that counts its calls and can be broken on demand.""" + + def __init__(self) -> None: + self.calls = 0 + self.broken = False + + def __call__(self, action): + self.calls += 1 + if self.broken: + raise RuntimeError("ctrlrun-test: the precondition provider is down") + return {"balance": 0} + + +def _strand_at_one(control, store, provider, remote): + """Attempt 1: ask, grant, dispatch, time out. Leaves the record AMBIGUOUS at 1.""" + with pytest.raises(ApprovalRequired) as asked: + _with_provider(control, remote, provider) + store.grant_approval(asked.value.request_id, "ops@example.com") + with pytest.raises(TimeoutError): + _with_provider(control, remote, provider, approval=asked.value.request_id) + assert store.get_effect(KEY).state is EffectState.AMBIGUOUS + + +def test_the_provider_runs_in_front_of_the_ceiling_on_the_reconcile_route(stores, fake_clock): + """§5.5's reconcile bullet, with the numbers it states (SPEC-v0.7 §5.5, §6.6). + + `_recheck` sits immediately before each `_take`, and this route takes twice, so a doomed + attempt under `max_attempts: 1` spends the operator's provider three times before the check + refuses: once on the request pass and twice on the retry. §6.6's principle, that a provider + is spent only where its answer can matter, does not reach the ceiling, and the spec says so + rather than closing it. + """ + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + provider = _Provider() + remote = Remote(TimeoutError("the response was lost"), "never reached") + _strand_at_one(control, store, provider, remote) + + before = provider.calls + hook = _Hook("not_executed") + with pytest.raises(ApprovalRequired) as asked: + _with_provider(control, remote, provider, reconcile=hook) + store.grant_approval(asked.value.request_id, "ops@example.com") + with pytest.raises(ActionDenied) as refused: + _with_provider(control, remote, provider, approval=asked.value.request_id, reconcile=hook) + + assert refused.value.reason == CEILING + assert provider.calls - before == 3, ( + "one call on the request pass and two on the retry, because _recheck runs per _take" + ) + assert remote.calls == 1, "and none of the three could have changed the answer" + + +def test_a_broken_provider_renames_the_refusal_of_an_attempt_that_could_never_run( + stores, fake_clock +): + """The sharper half of the same ordering: the operator is told the wrong reason. + + The provider runs first, so its failure is what refuses the call: `ApprovalMismatch` with + `precondition_unavailable`, not `ActionDenied` with `attempt_ceiling`. No effect record is + written, the record stays `AMBIGUOUS` at N, and the `reconcile` hook never runs. + """ + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + provider = _Provider() + remote = Remote(TimeoutError("the response was lost"), "never reached") + _strand_at_one(control, store, provider, remote) + + hook = _Hook("not_executed") + with pytest.raises(ApprovalRequired) as asked: + _with_provider(control, remote, provider, reconcile=hook) + store.grant_approval(asked.value.request_id, "ops@example.com") + + provider.broken = True + with pytest.raises(ApprovalMismatch) as refused: + _with_provider(control, remote, provider, approval=asked.value.request_id, reconcile=hook) + assert refused.value.reason == "precondition_unavailable" + assert hook.calls == 0, "the provider refused before the reconcile could run" + record = store.get_effect(KEY) + assert record.state is EffectState.AMBIGUOUS + assert record.attempt == 1, "nothing was reserved, so the ceiling never had an opinion" + assert remote.calls == 1 + + +def test_the_sequential_route_never_calls_the_provider(stores, fake_clock): + """§6.6's principle where the fast path can answer: zero calls, and the right reason.""" + store = stores() + control = _control(APPROVE_CEILING_1, store, fake_clock) + provider = _Provider() + remote = Remote(_not_executed(), "never reached") + + with pytest.raises(ApprovalRequired) as asked: + _with_provider(control, remote, provider) + store.grant_approval(asked.value.request_id, "ops@example.com") + with pytest.raises(NotExecuted): + _with_provider(control, remote, provider, approval=asked.value.request_id) + assert store.get_effect(KEY).state is EffectState.FAILED + + before = provider.calls + with pytest.raises(ActionDenied) as refused: + _with_provider(control, remote, provider) + assert refused.value.reason == CEILING + assert provider.calls == before, "the fast path refuses before the approval gate" + assert remote.calls == 1 + + # --- what the refused attempt number costs, and what the ceiling does not bound ---------- @@ -1545,8 +1717,16 @@ def test_T252_the_catalogue_is_in_id_order(tmp_path): """`BY_ID`'s insertion order is the report's order (SPEC-v0.7 §9.4). Items 3, 4 and 5 each appended after G13 on their own branch, so a textual merge yields - G13/G15/G14 as easily as a conflict, and nothing but this would notice: every count still - adds up and every id is still present, and the table simply prints out of order. + G13/G15/G14 as easily as a conflict, and the table then simply prints out of order while + every count still adds up and every id is still present. + + **This is a second guard, not the only one, and the difference matters.** An earlier + docstring here claimed nothing else would have noticed the swap; a review checked and it is + untrue. `tests/test_verify.py`'s catalogue test has asserted the same ordering since before + this branch existed, and it fails on the same mutation. What this one adds is locality: the + merge that could produce the swap is item 4's, and a reader of item 4's tests should find + the assertion that governs it here rather than in another file. `BY_ID`'s own order is the + part `test_verify.py` does not assert. """ from ctrlrun.verify import guarantees as reg