From e578a224d61b896f9490fef215133b4199493c3d Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 13:23:46 +0530 Subject: [PATCH 1/6] The approver is a principal, and an approval says who verified them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approval.approver is a string whose only check is that it is not empty, and adapter.py has always conceded what that string often is: a channel, wherever the framework's primitive does not identify a person. A deployment may now name an ApproverIdentity, and where one is named an approval is consumable only if the store holds a VerifiedApprover for it. Opt in, then fail closed, which is v0.3 §1.2's rule for authority applied to the approver. Without one, 0.7.0 exactly, asserted field by field across the whole approve-and-execute path. With one, no partial mode: an approval whose row carries no verified approver is refused, including one granted before the provider was configured, one granted through a surface that cannot resolve, and one held by a store that ignores the column. The check lives at the consumption because Control never grants an approval. Two things about that seam, both of which the spec review found before any code existed and both of which building it confirmed. _recheck returned early on every deployment not using preconditions, so a check added after that return would have been dead on the default path, green, and invisible to a mutation table. And the check is gated on a record that is granted and not lapsed by this clock, using check_consumable's own verdict: without the gate a denied approval reports approver_unverified, so a human's no stops appearing in the evidence as a no, and G1, G2 and the lapse write go with it. G18 grades the refusal of a self-approval, compared on the resolved principal and never on the string. Verify supplies the approver identity it grades against: whether an operator configured one is a fact about their application, which verify cannot see and which its own module forbids as an N/A reason. The operator MCP server is the one shipped surface that can produce a verified approver, and it has resolved a principal for every request since it shipped and then discarded it into mcp-operator:. Four lines stop discarding it. ctrlrun approve, the webhook and the adapters cannot, and SPEC-v0.8 §2.6 is the table that says so before anyone configures this. Needs ctrlrun.receipt/v5 and ctrlrun.guarantees/v4, each moving once, and migration 0006_verified_approver across all three stores, with approvers COLLATE "C" on Postgres because item 4 makes it a compare-and-set column. Eighteen tests outside this item moved, each a count or key set true of 0.7.0 and not now, and §14.2 lists them. The verify counts are pinned in .github/workflows/ci.yml as well, which would have turned that job red on a branch whose suite was green. Signed-off-by: arpan --- .github/workflows/ci.yml | 7 +- CHANGELOG.md | 33 ++ docs/SPEC-v0.8.md | 41 +++ src/ctrlrun/__init__.py | 4 + src/ctrlrun/approval.py | 152 ++++++++- src/ctrlrun/control.py | 86 ++++- src/ctrlrun/gateway/operator.py | 21 +- src/ctrlrun/migrations.py | 29 ++ src/ctrlrun/postgres.py | 34 +- src/ctrlrun/receipt.py | 55 ++- src/ctrlrun/state.py | 71 +++- src/ctrlrun/verify/guarantees.py | 15 +- src/ctrlrun/verify/scenarios.py | 129 ++++++- tests/test_approver.py | 553 +++++++++++++++++++++++++++++++ tests/test_attempt_cap.py | 2 +- tests/test_clock_skew.py | 2 +- tests/test_demo.py | 3 + tests/test_idempotency.py | 2 +- tests/test_mcp_operator.py | 39 +++ tests/test_observe.py | 2 +- tests/test_preconditions.py | 22 +- tests/test_protect.py | 5 + tests/test_verify.py | 45 ++- tests/test_verify_action.py | 12 +- tests/test_verify_authority.py | 6 +- tests/test_verify_report.py | 15 +- 26 files changed, 1311 insertions(+), 74 deletions(-) create mode 100644 tests/test_approver.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e015923f..6d406d2f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -120,12 +120,15 @@ jobs: set -eu echo "authority: $AUTHORITY ($AUTHORITY_NA not applicable)" echo "templates: $TEMPLATES ($TEMPLATES_NA not applicable)" - test "$AUTHORITY" = "verified 14/14" + test "$AUTHORITY" = "verified 15/15" # G13 is N/A on SQLite, the action's default store: SQLite has no clock of its own # to diverge from; G15 is N/A because neither document declares `max_attempts`. # G16 is graded on both: verify brings its own precondition provider (SPEC-v0.7 §8.9). + # G18 likewise since v0.8 item 2, which supplies its own approver identity: whether + # an operator configured one is a fact about their application and never an N/A + # reason here (SPEC-v0.8 §11.7). Both counts moved by one when it landed. test "$AUTHORITY_NA" = "2" - test "$TEMPLATES" = "verified 8/8" + test "$TEMPLATES" = "verified 9/9" test "$TEMPLATES_NA" = "8" test -s verify-badge.json test -s verify-report.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 85b7a213..e6e77501 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,39 @@ any change to one appears here. ### Added +- **The approver is a principal** (`docs/SPEC-v0.8.md` §2, §4.1). `Approval.approver` is a string + whose only check is that it is not empty, and `adapter.py` has always conceded what that string + often is: a channel, wherever the framework's primitive does not identify a person. A deployment + may now name an **`ApproverIdentity`**, and where one is named an approval is consumable only if + the store holds a **`VerifiedApprover`** for it: a principal the granting surface resolved, + recorded on the approval row, and carried onto the receipt. + + **Opt in, then fail closed**, which is `SPEC-v0.3.md` §1.2's rule for authority applied to the + approver. A `Control` built without one behaves exactly as 0.7.0 did, asserted field by field on + the whole approve-and-execute path. One built with it gets no partial mode: an approval whose row + carries no verified approver is refused with `approver_unverified`, **including one granted + before the provider was configured**, including one granted through a surface that cannot + resolve, and including one held by a store that ignores the column. + + **`ctrlrun verify` grades sixteen guarantees now**, G18 among them under + `ctrlrun.guarantees/v4`: an approval granted by the principal that requested the action is + refused, compared on the resolved principal and never on the string. Verify supplies the approver + identity it grades against, so what it reports is the kernel's refusal and never whether an + operator configured anything, which is a fact about their application and not about their + document. + + **Which surfaces can produce a verified approver, stated plainly because it is narrower than the + feature's name suggests.** The operator MCP server can, and now does: it has resolved a principal + for every request since it shipped and then discarded it into `mcp-operator:`. An embedding + application can. **`ctrlrun approve`, the webhook and the adapters cannot**, and the approvals + they grant are refused wherever an approver identity is configured. A deployment whose approvals + arrive through one of those three turns its approval path off by configuring this, which is the + rule working rather than a defect, and §2.6's table is the thing to read before configuring. + + Needs `ctrlrun.receipt/v5`, which adds `approvers` and `authority_grant_id`, and migration + `0006_verified_approver`. Every reader upgrades before any writer switches (`SPEC-v0.3.md` + §12.2). + - **`ctrlrun revoke --created-by PRINCIPAL` and `--under ID`** (`docs/SPEC-v0.8.md` §7). During an incident the operation an operator reaches for is *everything this principal issued* or *everything under this grant*, and until now that was a script over the events file, written diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 40471951..9e2f8844 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -1974,6 +1974,47 @@ text editor, which is the point `v0.3 §5.5` makes about evaluation. ### 14.2 Item 2: the approver is a principal +**The check is at the consumption because `Control` never grants**, which §1.4 recorded and which +building it confirmed: the only code that calls `grant_approval` outside a test is the CLI, the +operator server, `handle_inbound`, the scripted provider, the adapters, verify's own scenarios and +`Control._withdraw`. None of them is `Control` deciding anything. + +**The gate of §2.4.1 is `check_consumable` with its verdict's `record` tested and its `refusal` and +`expire` discarded.** It needed no new expiry logic, no second implementation of a frozen rule and +no new clock read: `control.py` already imports that function and already calls it twice. The +mutation table's M3 removes the gate and T291b's four rows go red together, which is what a gate +protecting four shipped reasons should do. + +**The early return was exactly as dangerous as §2.4 said.** M6 restores it, every approver test in +the file goes green, and only T291 fails: a check placed after that return is dead on the path +every 0.6-shaped deployment takes, and nothing else in the suite notices. + +**One test file covering one store is one store covered.** The first draft of `test_approver.py` +used the in-memory store alone, and the mutation table caught it: blanking the verified approver in +the **SQLite** write path left all twenty tests green, because none of them had ever executed that +path. The fixture now runs every test on in-memory, SQLite and Postgres, which is `v0.6 §2`'s +argument for the store conformance suite applied to a test file, and M7a and M7b are two rows +rather than one. + +**G18's title is 28 characters because the report table is 32 wide**, which v0.7 had to discover +for G12 as well. It is "the requester cannot approve" and not "self-approval is refused", because +what is compared is the resolved principal on each side and "self" invites the reading that two +different approver strings are two different people, which is the reading §4.1 exists to refuse. + +**What the two version bumps moved in the suite, listed rather than absorbed.** Eighteen tests +outside this item's own file changed, and every one of them was a count or a key set that was true +of 0.7.0 and is not true now: four pin the receipt's exact JSON key set, which `v5` widens by two; +eleven pin verify counts, because G18 is graded wherever a document sends an action to approval, so +the shipped examples move from 14/14 to 15/15 and from 8/8 to 9/9; one pins the last migration by +name, and now asserts `HEAD`; and the remaining two pin the receipt schema label this binary +writes. **The verify counts are also pinned in `.github/workflows/ci.yml`**, which would have +turned the `verify` job red on a branch whose suite was entirely green, and which nothing in the +local gate would have caught. + +**`_granting_principal` stayed package-internal and the operator server is its first caller.** That +server has resolved a principal for every request since it shipped and then discarded it into +`mcp-operator:`; item 2 is, on that surface, four lines that stop discarding it. + ### 14.3 Item 3: entitlement from the control registry ### 14.4 Item 4: M-of-N diff --git a/src/ctrlrun/__init__.py b/src/ctrlrun/__init__.py index 572b3f4c..bc7c60e1 100644 --- a/src/ctrlrun/__init__.py +++ b/src/ctrlrun/__init__.py @@ -16,8 +16,10 @@ Approval, ApprovalProvider, ApprovalRequest, + ApproverIdentity, LocalApprovalProvider, ScriptedApprovalProvider, + VerifiedApprover, ) from .authority import Authority, AuthorityResult, Delegation, Grant, Subject from .control import Control, context, idempotency_token, protect, with_approval @@ -63,6 +65,7 @@ "ApprovalRequest", "ApprovalRequired", "ApprovalTimeout", + "ApproverIdentity", "Authority", "AuthorityDenied", "AuthorityEscalation", @@ -105,6 +108,7 @@ "StaticIdentityProvider", "Subject", "Suspended", + "VerifiedApprover", "WebhookApprovalProvider", "action_hash", "banner", diff --git a/src/ctrlrun/approval.py b/src/ctrlrun/approval.py index e2b91639..13ee67d3 100644 --- a/src/ctrlrun/approval.py +++ b/src/ctrlrun/approval.py @@ -9,6 +9,7 @@ from __future__ import annotations import hashlib +import logging import secrets import time from collections.abc import Callable, Iterable, Iterator, Mapping @@ -19,7 +20,7 @@ from enum import StrEnum from typing import Any, Final, Protocol, runtime_checkable -from .action import Action, canonical_bytes +from .action import Action, Principal, canonical_bytes from .errors import ( ActionDenied, ApprovalMismatch, @@ -27,6 +28,9 @@ CTRLRunError, InvalidArgument, ) +from .identity import IdentityContext, IdentityProvider + +_LOG = logging.getLogger("ctrlrun") #: SPEC-v0.1 §4.1 — an approval request lives for fifteen minutes unless told otherwise. DEFAULT_APPROVAL_TTL: Final = timedelta(minutes=15) @@ -76,6 +80,147 @@ class ApprovalStatus(StrEnum): CONSUMED = "consumed" +#: SPEC-v0.8 §2.7, §4.1: the reasons an approver refusal carries. Values of the existing +#: `ApprovalMismatch.reason` field, because four refusals sharing a type is why every test +#: asserts the reason and never the type alone. +APPROVER_UNVERIFIED: Final = "approver_unverified" +APPROVER_IS_REQUESTER: Final = "approver_is_requester" + + +@dataclass(frozen=True) +class VerifiedApprover: + """Who answered, as the surface that took the answer verified them (SPEC-v0.8 §2.5). + + **No claim value is here and none ever will be.** `v0.3 §2.4`'s rule is that evidence + carries claim *names* where values are withheld, and what an entitlement decision means is + *which control this approver satisfied*, which is what `entitled` says. A row holding the + role value would put an identity provider's payload in an evidence table for no gain. + + `entitled` is filled by item 3 and is empty until then. + """ + + agent: str + user: str | None + issuer: str | None + granted_at: datetime + entitled: tuple[str, ...] = () + + def __post_init__(self) -> None: + if not self.agent: + raise InvalidArgument("a verified approver must carry a non-empty agent") + _require_aware(self.granted_at, "verified approver granted_at") + object.__setattr__(self, "entitled", tuple(self.entitled)) + + @property + def principal(self) -> tuple[str, str | None]: + """What `§4.1` compares: agent and user, and nothing else.""" + return (self.agent, self.user) + + def to_dict(self) -> dict[str, Any]: + return { + "agent": self.agent, + "user": self.user, + "issuer": self.issuer, + "entitled": list(self.entitled), + "granted_at": self.granted_at.isoformat(), + } + + @classmethod + def from_dict(cls, document: Mapping[str, Any]) -> VerifiedApprover: + return cls( + agent=str(document["agent"]), + user=document.get("user"), + issuer=document.get("issuer"), + granted_at=datetime.fromisoformat(str(document["granted_at"])), + entitled=tuple(document.get("entitled") or ()), + ) + + +@dataclass(frozen=True) +class ApproverIdentity: + """How a deployment verifies who answered an approval (SPEC-v0.8 §2.3). + + **Opt in, then fail closed.** A `Control` built without one behaves exactly as 0.7.0 did; + one built with it refuses any approval whose row carries no `VerifiedApprover`, wherever + that approval came from and whatever the store did with the column. + + A second instance of `v0.3`'s `IdentityProvider` and never the agent's: the agent's provider + reads what a proxy set for the agent, and a deployment where one object answers both doors + is one where the agent's own token can grant the agent's own approvals. + + `roles_claim` names the claim an issuer puts roles in. It is item 3's, and nothing reads it + yet; it lives here because a provider without it cannot answer §3, so a deployment that sets + one and forgets the other would have a silent half-check. + """ + + provider: IdentityProvider + roles_claim: str | None = None + + def __post_init__(self) -> None: + if self.roles_claim is not None and not self.roles_claim.strip(): + raise InvalidArgument("roles_claim must be a non-empty string or None") + if type(self.provider).__name__ == "StaticIdentityProvider": + # §2.3: a warning and not a refusal: a single-operator deployment where the shell + # genuinely is the human is real, and the record it produces is true. What is not + # true is that such a record distinguishes anybody, and an operator who has not + # thought about that should read it here rather than discover it in an audit. + _LOG.warning( + "the approver identity uses StaticIdentityProvider, which answers with one " + "name for every request: every approval it verifies will carry an identical " + "approver, and only self-approval can still be told apart (SPEC-v0.8 §2.3)" + ) + + def resolve(self, context: IdentityContext) -> Principal | None: + """The principal this door's provider verifies, or `None` where it declines. + + A decline is not backfilled from anything the calling code said: there is no `context()` + on this door to fall back to, and falling back would turn "nobody proved who this was" + into an approval (`v0.3 §3.2`). + """ + return self.provider.resolve(context) + + +#: SPEC-v0.8 §2.5: how a surface that resolved an approver hands that principal to the store +#: call that records it. **Package-internal on purpose** (§2.5.1): a public one would be an +#: unauthenticated way to assert a verified approver, which is `trust_approver` spelled as a +#: context manager. The shipped surfaces are its only callers, and the residual is stated in +#: §2.5.1 rather than hidden: anything inside the application's own process can call a private +#: function, so the kernel's claim is about what the shipped surfaces record. +_GRANTING_PRINCIPAL: ContextVar[tuple[Principal, tuple[str, ...]] | None] = ContextVar( + "ctrlrun_granting_principal", default=None +) + + +@contextmanager +def _granting_principal(principal: Principal, *, entitled: Iterable[str] = ()) -> Iterator[None]: + """Record `principal` as the verified approver of any grant made inside this block.""" + token = _GRANTING_PRINCIPAL.set((principal, tuple(entitled))) + try: + yield + finally: + _GRANTING_PRINCIPAL.reset(token) + + +def verified_approver_now(now: datetime) -> VerifiedApprover | None: + """The verified approver a store should record for a grant taken at `now`, if any. + + Read by the shipped stores inside `grant_approval` and `deny_approval`. A store that does + not read it records nothing, and `Control` then refuses every approval it granted, which is + the fail-closed direction and the reason the check lives at consumption (§2.4). + """ + found = _GRANTING_PRINCIPAL.get(None) + if found is None: + return None + principal, entitled = found + return VerifiedApprover( + agent=principal.agent, + user=principal.user, + issuer=principal.issuer, + granted_at=now, + entitled=entitled, + ) + + @dataclass(frozen=True) class ApprovalRequest: """A pending question for a human: may this exact action run? (SPEC-v0.1 §4.1)""" @@ -155,6 +300,11 @@ class ApprovalRecord: approver: str | None = None granted_at: datetime | None = None consumed_at: datetime | None = None + #: SPEC-v0.8 §2.5: every approver a resolving surface verified, in the order they answered. + #: Empty where the surface could not resolve one, which `Control` refuses at consumption + #: wherever an `ApproverIdentity` is configured (§2.7). Item 4 makes this list longer than + #: one; until then a granted record carries nought or one. + approvers: tuple[VerifiedApprover, ...] = () @property def approval_id(self) -> str: diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index e56be04c..e7bd4481 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -23,13 +23,17 @@ from .action import Action, Principal from .approval import ( + APPROVER_IS_REQUESTER, + APPROVER_UNVERIFIED, DEFAULT_APPROVAL_TTL, Approval, ApprovalProvider, ApprovalRecord, ApprovalRequest, ApprovalStatus, + ApproverIdentity, LocalApprovalProvider, + VerifiedApprover, _precondition_at_request, _precondition_fingerprint, check_consumable, @@ -363,19 +367,25 @@ class _Compared: failure by its type name, never its message: a provider that put the balance it read into its exception would otherwise carry raw state into the evidence through the one field nobody thought to check (§6.5). + + SPEC-v0.8 §2.5 adds `approvers` to it, because it is already the per-call scratch the + presenting pass fills and the receipt reads: the alternative was a second `get_approval` + per receipt, on a path that has just read the record. """ - __slots__ = ("at_recheck", "at_request", "error") + __slots__ = ("approvers", "at_recheck", "at_request", "error") def __init__(self, at_request: str | None = None) -> None: self.at_request = at_request self.at_recheck: str | None = None self.error: str | None = None + self.approvers: tuple[VerifiedApprover, ...] = () def reset(self) -> None: self.at_request = None self.at_recheck = None self.error = None + self.approvers = () def data(self) -> dict[str, Any]: data: dict[str, Any] = { @@ -479,6 +489,7 @@ def __init__( identity: IdentityProvider | None = None, authority: Authority | None = None, environment: str | None = None, + approver_identity: ApproverIdentity | None = None, ) -> None: self._policy = policy self._store = store @@ -492,6 +503,11 @@ def __init__( self._suspend_timeout = _checked_lease(suspend_timeout, "Control(suspend_timeout=...)") self._identity = identity self._authority = authority + #: SPEC-v0.8 §2.3: opt in, then fail closed. `None` is 0.7.0 exactly; anything else + #: makes an approval consumable only where the row carries a verified approver (§2.7). + #: `Control` never *resolves* one: it never grants an approval, so what it does with + #: this is check what the granting surface recorded (§1.4 item 1). + self._approver_identity = approver_identity #: SPEC-v0.6 §7.1's *"both are folded into the one canonical structure before hashing"*. #: `Policy` cannot see a separately-loaded `Authority` and this can, so the hash every #: receipt and every approval request carries is composed here. Where the authority came @@ -569,6 +585,11 @@ def identity(self) -> IdentityProvider | None: """The provider this Control resolves principals from, if any (SPEC-v0.3 §3.1).""" return self._identity + @property + def approver_identity(self) -> ApproverIdentity | None: + """How this deployment verifies who answered an approval, or `None` (SPEC-v0.8 §2.3).""" + return self._approver_identity + @property def authority(self) -> Authority | None: """The grants this Control evaluates against, if any (SPEC-v0.3 §4.1). @@ -2526,15 +2547,71 @@ def _recheck( compared.reset() record = self._store.get_approval(approval_id) stored = None if record is None else record.request.precondition_fingerprint + compared.at_request = stored + # SPEC-v0.8 §2.4: **the early return is gone.** It returned here whenever no provider + # was named and the record carried no fingerprint, which is every deployment that does + # not use `v0.7 §6`, and an approver check added after it would have been dead on that + # path, green, and invisible to a mutation table. + self._check_approver(action, approval_id, record, compared) if preconditions is None and stored is None: return - compared.at_request = stored verdict = check_consumable(record, approval_id, action.action_hash, self._clock()) if verdict.refusal is not None: raise verdict.refusal assert record is not None # a verdict with no refusal carries its record self._compare(action, record, preconditions, compared) + def _check_approver( + self, + action: Action, + approval_id: str, + record: ApprovalRecord | None, + compared: _Compared, + ) -> None: + """Who answered, and whether they may have (SPEC-v0.8 §2.7, §4.1). + + **Gated on a record that is `granted` and that this clock does not consider lapsed** + (§2.4.1). Everything else is left to the store, unchanged, and the reason is four rows + long: a denied approval carries no verified approver, so an ungated check would refuse + it `approver_unverified` and a human's no would stop appearing in the evidence as a no; + a consumed one is `G2`'s replayed approval and a moved hash is `G1`, both of which would + lose their reason; and a lapsed grant would lose `APPROVAL_EXPIRED` **and the store's + own lapse write**, because that write happens inside `_take` and a refusal raised here + never reaches it. + + The gate is `check_consumable`, the pure function `v0.1 §4.2` froze, with its `record` + tested and its `refusal` and `expire` discarded: no second implementation of a frozen + rule, no new clock read, and the store still decides expiry and may disagree. + """ + if record is not None: + # Recorded whatever this deployment checks, so a receipt says who answered even + # where no approver identity is configured and nothing was refused. + compared.approvers = record.approvers + if self._approver_identity is None: + return + verdict = check_consumable(record, approval_id, action.action_hash, self._clock()) + if verdict.record is None: + return + approvers = verdict.record.approvers + if not approvers: + raise ApprovalMismatch( + f"approval {approval_id} carries no verified approver, and this deployment " + "names an approver identity; the approval is left granted", + reason=APPROVER_UNVERIFIED, + approval_id=approval_id, + ) + requester = (action.principal.agent, action.principal.user) + for approver in approvers: + if approver.principal == requester: + # §4.1: on the resolved principal and never on the string, which is why two + # grants whose `approver` strings differ are still one principal here. + raise ApprovalMismatch( + f"approval {approval_id} was granted by {approver.agent!r}, which is the " + "principal that requested the action; the approval is left granted", + reason=APPROVER_IS_REQUESTER, + approval_id=approval_id, + ) + def _compare( self, action: Action, @@ -2859,6 +2936,7 @@ def _record( attempt: int = 1, observation: _Observation | None = None, compared: _Compared | None = None, + approvers: tuple[VerifiedApprover, ...] = (), ) -> Receipt: # SPEC-v0.3 §6.3 — one place turns a terminal outcome into an observed receipt, so # `result`, `execution` and `would_have` cannot disagree about the same action. The @@ -2896,6 +2974,10 @@ def _record( # there was none. precondition_at_request=None if compared is None else compared.at_request, precondition_at_recheck=None if compared is None else compared.at_recheck, + # SPEC-v0.8 §2.5: what §2 verified reaches the evidence, or the milestone records + # nothing. Read from the row rather than from the `Approval`, which carries only the + # string `v0.1 §4.1` froze. + approvers=approvers or (() if compared is None else compared.approvers), ) # The store assigns `seq`, `prev_hash` and `hash` (SPEC-v0.6 §6.2, §6.3), so what goes # to the sinks and back to the caller is the **chained** receipt. Handing the unchained diff --git a/src/ctrlrun/gateway/operator.py b/src/ctrlrun/gateway/operator.py index d2300b36..dbb4dbaa 100644 --- a/src/ctrlrun/gateway/operator.py +++ b/src/ctrlrun/gateway/operator.py @@ -34,7 +34,7 @@ from typing import Any, Final from ..action import Principal -from ..approval import ApprovalRecord, ApprovalStatus +from ..approval import ApprovalRecord, ApprovalStatus, _granting_principal from ..control import Control from ..effect import RESOLVED_BY_HUMAN, EffectState from ..errors import CTRLRunError, IdentityError, InvalidArgument @@ -764,12 +764,12 @@ def _write( ) -> dict[str, Any]: who = self._attribution(principal) if tool.name == "approve": - return self._approve(str(arguments["request_id"]), who) + return self._approve(str(arguments["request_id"]), who, principal) if tool.name == "deny": - return self._deny(str(arguments["request_id"]), who) + return self._deny(str(arguments["request_id"]), who, principal) return self._resolve_effect(arguments, who) - def _approve(self, request_id: str, who: str) -> dict[str, Any]: + def _approve(self, request_id: str, who: str, principal: Principal) -> dict[str, Any]: """The two calls `ctrlrun approve` makes, in the same order (§4.5). The record's `action_hash` is whatever was stored when the request was created (`v0.1 @@ -779,7 +779,13 @@ def _approve(self, request_id: str, who: str) -> dict[str, Any]: """ store = self.store record = store.get_approval(request_id) - approval = store.grant_approval(request_id, who) + # SPEC-v0.8 §2.6: **this server is the surface that can do this**, and until now it + # resolved a principal for every request and then discarded it into the string `who`. + # The principal its own provider verified is recorded beside that string, so an + # approval granted here is consumable in a deployment that checks (§2.7). It is recorded + # whether or not the deployment checks, because it is true either way. + with _granting_principal(principal): + approval = store.grant_approval(request_id, who) # `grant_approval` refuses an unknown id (`v0.1 §4.1`, `check_answerable`) and nothing # deletes an approval row, so the record exists by here. This is an invariant check and # not a guard against a caller: the previous spelling, `if record is not None:`, was a @@ -808,10 +814,11 @@ def _approve(self, request_id: str, who: str) -> dict[str, Any]: "expires_at": iso_timestamp(approval.expires_at), } - def _deny(self, request_id: str, who: str) -> dict[str, Any]: + def _deny(self, request_id: str, who: str, principal: Principal) -> dict[str, Any]: store = self.store record = store.get_approval(request_id) - store.deny_approval(request_id, who) + with _granting_principal(principal): + store.deny_approval(request_id, who) if record is None: # pragma: no cover - deny_approval refused an unknown id above raise _Refused( _INTERNAL_ERROR, diff --git a/src/ctrlrun/migrations.py b/src/ctrlrun/migrations.py index 1fad644e..4406b4c9 100644 --- a/src/ctrlrun/migrations.py +++ b/src/ctrlrun/migrations.py @@ -308,6 +308,30 @@ def sql(self, dialect: str) -> tuple[str, ...]: 'ALTER TABLE approvals ADD COLUMN IF NOT EXISTS precondition_fingerprint TEXT COLLATE "C"', ) +#: SPEC-v0.8 §11.1: what a verified approver is recorded in, and the two columns the request +#: pins for items 3 and 4. Three columns and one migration, because they are one change to one +#: table and a store has no use for a half of it. +#: +#: **`approvers` is `COLLATE "C"` on Postgres and the reason is not cosmetic.** Item 4 makes it +#: a compare-and-set column, and a non-deterministic collation can make two distinct blobs +#: compare equal, which fails the *unsafe* way: a compare-and-set that wrongly matches succeeds, +#: and the lost update the CAS exists to close comes straight back, on exactly the deployments +#: whose `lc_collate` is an ICU locale (§4.3). +#: +#: Nullable and **not backfilled**: every approval granted before this migration was granted by +#: a surface that recorded no principal, which is exactly what `NULL` means, and which +#: `Control` refuses at consumption wherever an approver identity is configured (§2.9). +_VERIFIED_APPROVER: Final = ( + "ALTER TABLE approvals ADD COLUMN approvers TEXT", + "ALTER TABLE approvals ADD COLUMN required_roles TEXT", + "ALTER TABLE approvals ADD COLUMN approvals_required INTEGER", +) +_VERIFIED_APPROVER_PG: Final = ( + 'ALTER TABLE approvals ADD COLUMN IF NOT EXISTS approvers TEXT COLLATE "C"', + 'ALTER TABLE approvals ADD COLUMN IF NOT EXISTS required_roles TEXT COLLATE "C"', + "ALTER TABLE approvals ADD COLUMN IF NOT EXISTS approvals_required INTEGER", +) + #: The ordered set this binary knows. `NNNN_snake_name`: four digits, zero-padded, so #: lexicographic order is application order. MIGRATIONS: Final[tuple[Migration, ...]] = ( @@ -320,6 +344,11 @@ def sql(self, dialect: str) -> tuple[str, ...]: _PRECONDITION_FINGERPRINT, postgres=_PRECONDITION_FINGERPRINT_PG, ), + Migration( + "0006_verified_approver", + _VERIFIED_APPROVER, + postgres=_VERIFIED_APPROVER_PG, + ), ) HEAD: Final = MIGRATIONS[-1].id diff --git a/src/ctrlrun/postgres.py b/src/ctrlrun/postgres.py index adbb7557..c7887438 100644 --- a/src/ctrlrun/postgres.py +++ b/src/ctrlrun/postgres.py @@ -48,6 +48,7 @@ ApprovalStatus, check_answerable, check_consumable, + verified_approver_now, ) from .effect import ( COMMITTED_EFFECT, @@ -78,6 +79,8 @@ _action_from_json, _action_json, _approver, + _approvers_from_json, + _approvers_json, _at, _checked, _iso, @@ -693,7 +696,8 @@ def _read_approval(self, connection: Any, approval_id: str) -> ApprovalRecord | cursor.execute( "SELECT approval_id, action_hash, status, action_json, approver, created_at, " "granted_at, expires_at, consumed_at, policy_hash_at_approval, " - f"precondition_fingerprint FROM {self._q}.approvals WHERE approval_id = %s", + "precondition_fingerprint, approvers " + f"FROM {self._q}.approvals WHERE approval_id = %s", (approval_id,), ) row = cursor.fetchone() @@ -713,6 +717,7 @@ def _read_approval(self, connection: Any, approval_id: str) -> ApprovalRecord | approver=row[4], granted_at=_at(row[6]), consumed_at=_at(row[8]), + approvers=_approvers_from_json(None if row[11] is None else str(row[11])), ) # --- reservation (SPEC-v0.6 §4.2) --------------------------------------------------- @@ -1516,20 +1521,29 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: self._use_schema(connection) try: record = self._answerable(connection, approval_id, now) + # SPEC-v0.8 §2.5: the verified approver the granting surface resolved, appended to + # whatever the row already holds. + verified = verified_approver_now(now) + approvers = (*record.approvers, verified) if verified else record.approvers granted = replace( - record, status=ApprovalStatus.GRANTED, approver=approver, granted_at=now + record, + status=ApprovalStatus.GRANTED, + approver=approver, + granted_at=now, + approvers=approvers, ) with connection.cursor() as cursor: # Conditional on what `check_answerable` saw. Unconditional, a concurrent # `deny_approval` was silently overwritten and `find_granted_approval` then # returned an approval a human had refused. cursor.execute( - f"UPDATE {self._q}.approvals SET status=%s, approver=%s, granted_at=%s " - "WHERE approval_id=%s AND status=%s", + f"UPDATE {self._q}.approvals SET status=%s, approver=%s, granted_at=%s, " + "approvers=%s WHERE approval_id=%s AND status=%s", ( str(ApprovalStatus.GRANTED), approver, _iso(now), + _approvers_json(approvers), approval_id, str(record.status), ), @@ -1553,11 +1567,19 @@ def deny_approval(self, approval_id: str, approver: str) -> None: self._use_schema(connection) try: record = self._answerable(connection, approval_id, now) + verified = verified_approver_now(now) + approvers = (*record.approvers, verified) if verified else record.approvers with connection.cursor() as cursor: cursor.execute( - f"UPDATE {self._q}.approvals SET status=%s, approver=%s " + f"UPDATE {self._q}.approvals SET status=%s, approver=%s, approvers=%s " "WHERE approval_id=%s AND status=%s", - (str(ApprovalStatus.DENIED), approver, approval_id, str(record.status)), + ( + str(ApprovalStatus.DENIED), + approver, + _approvers_json(approvers), + approval_id, + str(record.status), + ), ) if cursor.rowcount != 1: raise ApprovalMismatch( diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index c1d59d89..ab131365 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -22,6 +22,11 @@ from typing import Any, Final, Protocol from .action import Principal, canonical_bytes + +# `approval.py` imports `action`, `errors` and `identity` and nothing else, so this is +# downward (ARCHITECTURE §6): a receipt records what an approval verified, and the record +# type it records is that module's. +from .approval import VerifiedApprover from .errors import CTRLRunError, InvalidArgument from .policy import Decision @@ -35,12 +40,13 @@ #: SPEC-v0.7 §6.11. `v4` adds `precondition_at_request` and `precondition_at_recheck`, and it is #: the first bump that does not rehash every older receipt: a receipt read from a store is #: hashed as the document it was read from, and renders under its own schema's label and keys. -RECEIPT_SCHEMA: Final = "ctrlrun.receipt/v4" +RECEIPT_SCHEMA: Final = "ctrlrun.receipt/v5" _V1: Final = "ctrlrun.receipt/v1" _V2: Final = "ctrlrun.receipt/v2" _V3: Final = "ctrlrun.receipt/v3" _V4: Final = "ctrlrun.receipt/v4" +_V5: Final = "ctrlrun.receipt/v5" #: SPEC-v0.7 §6.11: each schema's top-level key set, exactly its released writers': `v1`, 19 #: keys, by 0.1.0 and 0.2.0; `v2`, 21, by 0.3.0rc1 to 0.5.0; `v3`, 26, by 0.6.0 and 0.6.1; @@ -69,7 +75,11 @@ _V2_KEYS: Final = (*_V1_KEYS[:16], "execution", "would_have", *_V1_KEYS[16:]) _V3_KEYS: Final = (*_V2_KEYS, "seq", "prev_hash", "policy_hash", "policy_version", "controls") _V4_KEYS: Final = (*_V3_KEYS, "precondition_at_request", "precondition_at_recheck") -_KEYS: Final = {_V1: _V1_KEYS, _V2: _V2_KEYS, _V3: _V3_KEYS, _V4: _V4_KEYS} +#: SPEC-v0.8 §11.3: `v5`, 30 keys. The whole shape is frozen before item 2 writes it, so a +#: reader can parse a `v5` receipt from any later item: `authority_grant_id` is item 5's and is +#: `None` until then, which is what "absent or null" means for a field nothing has filled. +_V5_KEYS: Final = (*_V4_KEYS, "approvers", "authority_grant_id") +_KEYS: Final = {_V1: _V1_KEYS, _V2: _V2_KEYS, _V3: _V3_KEYS, _V4: _V4_KEYS, _V5: _V5_KEYS} #: The two files of SPEC-v0.1 §6, written beside the state database. #: SPEC-v0.6 §6.2. The `prev_hash` of receipt 1, and the hash the head row starts at (§3.7), so @@ -372,6 +382,14 @@ class Receipt: #: document, so no reader surfaces the value of a key a document's schema does not declare. precondition_at_request: str | None = None precondition_at_recheck: str | None = None + #: SPEC-v0.8 §2.5: every approver a resolving surface verified for the approval this action + #: consumed. Empty where none was, which is every 0.7.0 deployment and every surface §2.6 + #: names as unable to resolve. Read only from a `v5` document. + approvers: tuple[VerifiedApprover, ...] = () + #: SPEC-v0.8 §5.4: the grant that decided this action, for **every** grant and not only for + #: break-glass. Item 5 fills it; `None` until then, which is the "absent or null" §11.4's + #: frozen shape promises a reader. + authority_grant_id: str | None = None #: The schema this receipt is written under (§6.11). A receipt this binary builds is #: `RECEIPT_SCHEMA`; one read from a store keeps the label its document declared, or `""` #: where it declared none, which renders with no `schema` key at all. @@ -457,6 +475,8 @@ def _full_document(self) -> dict[str, Any]: "controls": list(self.controls), "precondition_at_request": self.precondition_at_request, "precondition_at_recheck": self.precondition_at_recheck, + "approvers": [approver.to_dict() for approver in self.approvers], + "authority_grant_id": self.authority_grant_id, } def to_json(self) -> str: @@ -524,11 +544,18 @@ def from_dict(cls, document: Mapping[str, Any]) -> Receipt: policy_version=document.get("policy_version"), controls=_controls_of(document.get("controls")), precondition_at_request=( - document.get("precondition_at_request") if schema == _V4 else None + document.get("precondition_at_request") if schema in (_V4, _V5) else None ), precondition_at_recheck=( - document.get("precondition_at_recheck") if schema == _V4 else None + document.get("precondition_at_recheck") if schema in (_V4, _V5) else None ), + # SPEC-v0.8 §2.5, and `v0.7 §6.11`'s rule for a key a document's schema does not + # declare: read it only from a `v5` document, so no reader surfaces a field an older + # writer never wrote. **Never raises**, whatever the column holds: `from_dict` is + # the one function every reader of a chain goes through, and a raise on one tampered + # row would blind every reader at once. + approvers=_approvers_of(document.get("approvers")) if schema == _V5 else (), + authority_grant_id=document.get("authority_grant_id") if schema == _V5 else None, schema=schema, ) @@ -539,6 +566,26 @@ def from_json(cls, line: str) -> Receipt: return cls.from_dict(document) +def _approvers_of(value: object) -> tuple[VerifiedApprover, ...]: + """`Receipt.approvers` out of a document, never raising (SPEC-v0.8 §2.5, `v0.7 §6.11`). + + A malformed entry is dropped rather than thrown, on the rule `from_dict` already follows: + one tampered row must not blind every reader of the chain. What a dropped entry costs is + visible, because the receipt then shows fewer approvers than the row that produced it. + """ + if not isinstance(value, list): + return () + found = [] + for item in value: + if not isinstance(item, Mapping): + continue + try: + found.append(VerifiedApprover.from_dict(item)) + except (KeyError, ValueError, TypeError, InvalidArgument): + continue + return tuple(found) + + def _document_hash(document: Mapping[str, Any]) -> str: """`"sha256:" + hex(SHA-256(canonical_bytes(document)))`, the chain's one hash (§6.2). diff --git a/src/ctrlrun/state.py b/src/ctrlrun/state.py index 120707db..77f456e3 100644 --- a/src/ctrlrun/state.py +++ b/src/ctrlrun/state.py @@ -37,8 +37,10 @@ ApprovalRequest, ApprovalStatus, ApprovalStore, + VerifiedApprover, check_answerable, check_consumable, + verified_approver_now, ) from .effect import ( COMMITTED_EFFECT, @@ -733,6 +735,26 @@ def close(self) -> None: ... +def _approvers_json(approvers: tuple[VerifiedApprover, ...]) -> str | None: + """The verified approvers as one canonical JSON array, or `None` where there are none. + + `None` and not `"[]"`: a row granted by a surface that resolved nobody and a row granted + before the column existed are the same thing to a reader, and both are what `NULL` means + (SPEC-v0.8 §2.5). + """ + if not approvers: + return None + return json.dumps([approver.to_dict() for approver in approvers], sort_keys=True) + + +def _approvers_from_json(text: str | None) -> tuple[VerifiedApprover, ...]: + """What the column holds, or `()`. A column a store dropped reads as no approver at all, + which `Control` refuses at consumption rather than skipping (SPEC-v0.8 §2.5).""" + if not text: + return () + return tuple(VerifiedApprover.from_dict(item) for item in json.loads(text)) + + class InMemoryStateStore: """Everything held in process memory: for tests and `ctrlrun demo`. @@ -872,11 +894,17 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: approver = _approver(approver) with self._lock: record = self._answerable(approval_id) + now = self._clock() + # SPEC-v0.8 §2.5: whatever the granting surface verified, or nothing where it + # verified nobody. A store that did not read this records no approver, and that is + # refused at consumption rather than skipped. + verified = verified_approver_now(now) granted = replace( record, status=ApprovalStatus.GRANTED, approver=approver, - granted_at=self._clock(), + granted_at=now, + approvers=(*record.approvers, verified) if verified else record.approvers, ) self._approvals[approval_id] = granted return granted.as_approval() @@ -885,8 +913,12 @@ def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) with self._lock: record = self._answerable(approval_id) + verified = verified_approver_now(self._clock()) self._approvals[approval_id] = replace( - record, status=ApprovalStatus.DENIED, approver=approver + record, + status=ApprovalStatus.DENIED, + approver=approver, + approvers=(*record.approvers, verified) if verified else record.approvers, ) def consume_approval(self, approval_id: str, action_hash: str) -> Approval: @@ -1542,12 +1574,27 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: connection.execute("BEGIN IMMEDIATE") try: record = self._answerable(connection, approval_id, now) + # SPEC-v0.8 §2.5: the verified approver, appended to whatever the row already + # holds, inside the same `BEGIN IMMEDIATE` that serialises the status transition. + verified = verified_approver_now(now) + approvers = (*record.approvers, verified) if verified else record.approvers granted = replace( - record, status=ApprovalStatus.GRANTED, approver=approver, granted_at=now + record, + status=ApprovalStatus.GRANTED, + approver=approver, + granted_at=now, + approvers=approvers, ) connection.execute( - "UPDATE approvals SET status=?, approver=?, granted_at=? WHERE approval_id=?", - (str(ApprovalStatus.GRANTED), approver, _iso(now), approval_id), + "UPDATE approvals SET status=?, approver=?, granted_at=?, approvers=? " + "WHERE approval_id=?", + ( + str(ApprovalStatus.GRANTED), + approver, + _iso(now), + _approvers_json(approvers), + approval_id, + ), ) except BaseException: self._unwind(connection) @@ -1561,10 +1608,17 @@ def deny_approval(self, approval_id: str, approver: str) -> None: now = self._clock() connection.execute("BEGIN IMMEDIATE") try: - self._answerable(connection, approval_id, now) + record = self._answerable(connection, approval_id, now) + verified = verified_approver_now(now) + approvers = (*record.approvers, verified) if verified else record.approvers connection.execute( - "UPDATE approvals SET status=?, approver=? WHERE approval_id=?", - (str(ApprovalStatus.DENIED), approver, approval_id), + "UPDATE approvals SET status=?, approver=?, approvers=? WHERE approval_id=?", + ( + str(ApprovalStatus.DENIED), + approver, + _approvers_json(approvers), + approval_id, + ), ) except BaseException: self._unwind(connection) @@ -1610,6 +1664,7 @@ def _read_approval( approver=row["approver"], granted_at=_at(row["granted_at"]), consumed_at=_at(row["consumed_at"]), + approvers=_approvers_from_json(row["approvers"]), ) def _expire_locked(self, connection: sqlite3.Connection, approval_id: str) -> None: diff --git a/src/ctrlrun/verify/guarantees.py b/src/ctrlrun/verify/guarantees.py index 7ccd86d2..2f278c42 100644 --- a/src/ctrlrun/verify/guarantees.py +++ b/src/ctrlrun/verify/guarantees.py @@ -21,7 +21,10 @@ #: to tell a new guarantee from a corrupted line. #: SPEC-v0.7 §9.4: `v3` is G1 to G16. It moves once, with G13, and the other four join it as #: their items land; nothing is released in between. -CATALOGUE: Final = "ctrlrun.guarantees/v3" +#: SPEC-v0.8 §11.4: `v4` is G1 to G21, and it moves once, here, with G18. G17, G19, G20 and G21 +#: join it with their items, and item 8 asserts all five present before the release. No stub +#: rows: a guarantee that reports anything before its check exists is a false green. +CATALOGUE: Final = "ctrlrun.guarantees/v4" @dataclass(frozen=True) @@ -104,6 +107,16 @@ class Guarantee: "a moved fingerprint is refused", ("v0.1 §4.2", "v0.7 §8 T253", "v0.7 §8 T254"), ), + Guarantee( + "G18", + # 28 characters, because `report._TITLE_WIDTH` is 32 and a wider title breaks the + # table's alignment: v0.7 had to shorten G12's for the same reason. "the requester + # cannot approve" and not "self-approval is refused", because what is compared is the + # resolved principal on each side and never a string, and "self" invites the reading + # that two different strings are two different people (SPEC-v0.8 §4.1). + "the requester cannot approve", + ("v0.3 §4.2", "v0.8 §10 T285", "v0.8 §10 T286"), + ), ) #: By id, for `--only` and for the report. Insertion order is catalogue order. diff --git a/src/ctrlrun/verify/scenarios.py b/src/ctrlrun/verify/scenarios.py index 2f4abd0c..25953c1d 100644 --- a/src/ctrlrun/verify/scenarios.py +++ b/src/ctrlrun/verify/scenarios.py @@ -42,7 +42,13 @@ from uuid import uuid4 from ..action import Action, Principal -from ..approval import DEFAULT_APPROVAL_TTL, ApprovalStatus, LocalApprovalProvider +from ..approval import ( + DEFAULT_APPROVAL_TTL, + ApprovalStatus, + ApproverIdentity, + LocalApprovalProvider, + _granting_principal, +) from ..authority import ( AUTHORITY_EXPIRED, CONTAINMENT, @@ -75,6 +81,7 @@ NotExecuted, PolicyError, ) +from ..identity import IdentityContext from ..policy import Condition, Decision, Policy, _ActionPolicy, _Rule, discover_policy_path from ..receipt import ( BLOCKED_ATTEMPT_CEILING, @@ -105,6 +112,13 @@ #: and named so no reader of the evidence mistakes it for one. APPROVER: Final = "ctrlrun-verify" +#: SPEC-v0.8 §11.7: the approver identity G18 grades against. Verify builds its own scenarios, +#: so it supplies the provider too; what it grades is the kernel's refusal, never whether the +#: operator configured one, which is a fact about a constructor call and not about a document. +#: +#: Not `StaticIdentityProvider`: that one warns, by design (`v0.3 §3.3`), and a passing verify +#: run writes no kernel warning to stderr. + #: §3.6 — the base instant where no grant carries an `expires_at`. FALLBACK_T0: Final = datetime(2026, 1, 1, tzinfo=UTC) @@ -546,6 +560,16 @@ def _from_pattern(pattern: str | None) -> str | None: return pattern +@dataclass(frozen=True) +class _VerifyApproverProvider: + """An `IdentityProvider` answering one principal, for G18's own scenario (SPEC-v0.8 §11.7).""" + + principal: Principal + + def resolve(self, context: IdentityContext) -> Principal | None: + return self.principal + + class Engine: """Derives and runs the scenarios for one configuration (§3). @@ -836,7 +860,12 @@ def control( return control, store, recorder, moving def _control_for( - self, gid: str, selection: _Selection, *, clock: _Clock | None = None + self, + gid: str, + selection: _Selection, + *, + clock: _Clock | None = None, + approver_identity: ApproverIdentity | None = None, ) -> tuple[Control, StateStore, _Recorder, _Clock]: moving = clock if clock is not None else _Clock(self._t0) store, _ = self._store_for(gid, moving) @@ -849,6 +878,10 @@ def _control_for( sinks=[recorder], authority=self.authority, environment=selection.environment, + # SPEC-v0.8 §11.7: verify configures the approver identity it grades against, and + # only where a scenario asks for one. Every other scenario keeps the 0.7.0 shape, + # which is what keeps `ctrlrun verify` green on a deployment that verifies nobody. + approver_identity=approver_identity, ) return control, store, recorder, moving @@ -2985,6 +3018,98 @@ def body(detail: dict[str, Any]) -> None: finally: store.close() + # --- G18: an approver who is the requester is refused --------------------------------- + + def g18(self) -> GuaranteeResult: + """SPEC-v0.8 §4.1, §11.7. Graded wherever the document sends an action to approval. + + **Verify supplies the approver identity, and that is the point.** Whether the operator + configured one is a fact about a constructor call in their application, which verify + cannot see and which `verify/guarantees.py` forbids as an `N/A` reason: every reason + there is a statement about the operator's *document*. So the `N/A` here is the one G1 + and G2 already use, that no action requires approval, and it is true of the document. + + Both halves, `v0.4 §1.3`'s rule. The observable: an approval granted by the principal + that requested the action is refused, and the executor is not reached. The control: the + same action, approved by a different principal, commits. A check that refused every + approval would pass the first and fail the second. + + **The strings differ and the principals are the same**, which is what makes this a test + of §4.1's comparison rather than of a string. Both grants carry `APPROVER` as the + `approver` string; what differs is the principal the grant recorded. + """ + selection = self.select(decisions=(Decision.APPROVE,)) + if selection is None: + return self.na("G18", self.unselected(reg.NO_APPROVE_RULE)) + requester = selection.principal + identity = ApproverIdentity(_VerifyApproverProvider(requester)) + control, store, recorder, _ = self._control_for( + "G18", selection, approver_identity=identity + ) + + def body(detail: dict[str, Any]) -> None: + action = selection.build() + detail["approved_by_verify"] = True + detail["approver_identity"] = "supplied by verify (SPEC-v0.8 §11.7)" + request = control.approvals.request(action, DEFAULT_APPROVAL_TTL) + with _granting_principal(requester): + store.grant_approval(request.request_id, APPROVER) + executor = _Executor() + refusal = self.refused( + lambda: self.execute( + control, action, executor, selection.effect_key, request.request_id + ), + (ApprovalMismatch,), + "ApprovalMismatch(reason='approver_is_requester')", + "an action ran under an approval granted by the principal that requested it", + ) + reason = getattr(refusal, "reason", "") + _expect( + reason == "approver_is_requester", + "ApprovalMismatch(reason='approver_is_requester')", + f"ApprovalMismatch(reason={reason!r})", + ) + _expect( + executor.calls == 0, + "the executor is not reached", + f"the executor was called {executor.calls} times", + ) + record = store.get_approval(request.request_id) + _expect( + record is not None and record.status is ApprovalStatus.GRANTED, + "the approval is left granted and not consumed", + f"the approval is {None if record is None else record.status}", + ) + _expect( + _named_event( + recorder, EventType.APPROVAL_INVALIDATED, reason="approver_is_requester" + ), + "APPROVAL_INVALIDATED with reason 'approver_is_requester'", + f"events were {recorder.types()}", + ) + + # The control: a different principal, the same approver string, and it commits. + other = Principal( + agent=f"{requester.agent}-approver", user=requester.user, issuer=requester.issuer + ) + second = control.approvals.request(selection.build(), DEFAULT_APPROVAL_TTL) + with _granting_principal(other): + store.grant_approval(second.request_id, APPROVER) + committed = _Executor() + receipt = self.execute( + control, action, committed, selection.effect_key, second.request_id + ) + _expect_control( + receipt.result is ReceiptResult.COMMITTED and committed.calls == 1, + "the same action, approved by a different principal, commits", + f"it ended {receipt.result} after {committed.calls} executor calls", + ) + + try: + return self.graded("G18", selection, store, recorder, body) + finally: + store.close() + #: G12's loopback address: the literal, never `localhost` and never `::1` (SPEC-v0.7 §8.9). _LOOPBACK: Final = "127.0.0.1" diff --git a/tests/test_approver.py b/tests/test_approver.py new file mode 100644 index 00000000..e0592f16 --- /dev/null +++ b/tests/test_approver.py @@ -0,0 +1,553 @@ +"""T281 to T296: the approver is a principal (SPEC-v0.8 §2, §4.1). + +Opt in, then fail closed. A `Control` with no `ApproverIdentity` is 0.7.0 exactly; one that +names an identity refuses an approval whose row carries no verified approver, wherever the +approval came from and whatever the store did with the column. +""" + +from __future__ import annotations + +import json +import os +import uuid +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest + +from ctrlrun.action import Action, Principal +from ctrlrun.approval import ( + ApproverIdentity, + _granting_principal, +) +from ctrlrun.control import Control, with_approval +from ctrlrun.errors import ActionDenied, ApprovalMismatch, ApprovalRequired, IdentityError +from ctrlrun.identity import IdentityContext, StaticIdentityProvider +from ctrlrun.policy import Policy +from ctrlrun.receipt import RECEIPT_SCHEMA, EventType +from ctrlrun.state import InMemoryStateStore, SQLiteStateStore + +POLICY = """ +schema: ctrlrun.policy/v1 +actions: + payments.refund: + decision: approve + payments.read: + decision: allow +""" + +KEY = "refund:EU-42" + +UNVERIFIED = "approver_unverified" +IS_REQUESTER = "approver_is_requester" + +AGENT = Principal(agent="ops-agent", user="ada") + + +class _Clock: + def __init__(self) -> None: + self.now = datetime(2026, 9, 12, 10, 0, tzinfo=UTC) + + def __call__(self) -> datetime: + return self.now + + def advance(self, delta: timedelta) -> None: + self.now += delta + + +class _Recording: + """An `IdentityProvider` that answers a fixed principal and keeps what it was asked.""" + + def __init__(self, principal: Principal | None = None, raises: Exception | None = None) -> None: + self.principal = principal + self.raises = raises + self.contexts: list[IdentityContext] = [] + + def resolve(self, context: IdentityContext) -> Principal | None: + self.contexts.append(context) + if self.raises is not None: + raise self.raises + return self.principal + + +class _Executor: + def __init__(self) -> None: + self.calls = 0 + + def __call__(self) -> str: + self.calls += 1 + return "done" + + +@pytest.fixture +def clock(): + return _Clock() + + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + + +@pytest.fixture( + params=[ + "in-memory", + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + not POSTGRES_URL, + reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against", + ), + ), + ] +) +def store(request, clock, tmp_path): + """Every shipped store, because the `approvers` column is written by each of them. + + The first draft of this file used the in-memory store alone, and the mutation table caught + it: blanking the verified approver in the SQLite write path left every test green, because + nothing here had ever executed that path. A store's column is not covered by a test of a + different store (SPEC-v0.6 §2's argument for the conformance suite, applied to a test file). + """ + if request.param == "in-memory": + made = InMemoryStateStore(clock=clock) + elif request.param == "sqlite": + made = SQLiteStateStore(tmp_path / "state.db", clock=clock) + else: + from ctrlrun.postgres import PostgresStateStore + + schema = f"approver_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + made = PostgresStateStore(POSTGRES_URL, schema=schema, clock=clock) + yield made + made.close() + if request.param == "postgres": + from ctrlrun.postgres import PostgresStateStore + + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + +def _control(store, clock, *, approver_identity=None, principal=AGENT): + return Control( + Policy.from_yaml(POLICY), + store, + clock=clock, + approver_identity=approver_identity, + identity=StaticIdentityProvider(agent=principal.agent, user=principal.user), + ) + + +def _action(control, name: str = "payments.refund", **arguments: Any) -> Action: + return Action( + name=name, + arguments=arguments or {"amount": 100, "payment_id": "EU-42"}, + principal=AGENT, + environment=control.environment, + ) + + +def _requested(control, action, key: str | None = KEY) -> str: + with pytest.raises(ApprovalRequired) as pending: + control.execute(action, _Executor(), key) + return pending.value.request_id + + +def _present(control, action, request_id, executor=None, key: str | None = KEY): + with with_approval(request_id): + return control.execute(action, executor or _Executor(), key) + + +#: A verified approver, recorded the way a resolving surface records one (§2.5). +APPROVER = Principal(agent="human:bob", user="bob@example.com", issuer="https://issuer.example") + + +def _grant_verified(store, request_id, principal=APPROVER, approver="mcp-operator:bob"): + with _granting_principal(principal): + return store.grant_approval(request_id, approver) + + +# --- T281: an approval with no verified approver is refused ------------------------------------ + + +def test_T281_an_approval_with_no_verified_approver_is_refused(store, clock): + """THE test. The row was granted by a surface that cannot resolve, and it is not consumable.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + store.grant_approval(request_id, "cli:local") + executor = _Executor() + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id, executor) + + assert refused.value.reason == UNVERIFIED + assert executor.calls == 0 + record = store.get_approval(request_id) + assert record is not None + assert str(record.status) == "granted", "the human's yes is not spent on a refusal" + assert record.approvers == () + assert store.get_effect(KEY) is None, "nothing was reserved" + + +# --- T282: the positive control, with no ApproverIdentity at all ------------------------------- + + +def test_T282_with_no_approver_identity_the_whole_path_is_0_7_0(store, clock): + """R1: a deployment that names no approver identity is unchanged, field for field.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + store.grant_approval(request_id, "cli:local") + executor = _Executor() + + receipt = _present(control, action, request_id, executor) + + assert executor.calls == 1 + assert str(receipt.result) == "committed" + assert receipt.approver == "cli:local" + assert receipt.approval_id == request_id + assert receipt.approvers == () + assert store.get_approval(request_id).status == "consumed" + + +# --- T283: what a resolving surface records ---------------------------------------------------- + + +def test_T283_a_resolving_surface_records_the_principal_and_no_claim_value(store, clock): + """§2.5: agent, user and issuer reach the row; a claim value reaches nothing.""" + sentinel = "SENTINEL-CLAIM-VALUE" + principal = Principal( + agent="human:bob", + user="bob@example.com", + issuer="https://issuer.example", + claims={"roles": sentinel}, + ) + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(principal))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id, principal) + + receipt = _present(control, action, request_id) + + record = store.get_approval(request_id) + assert [(who.agent, who.user, who.issuer) for who in record.approvers] == [ + ("human:bob", "bob@example.com", "https://issuer.example") + ] + written = json.dumps( + [receipt.to_dict(), *[event.to_dict() for event in store.events()]], default=str + ) + assert sentinel not in written + assert sentinel not in json.dumps( + [approver.__dict__ for approver in record.approvers], default=str + ) + + +# --- T284: the receipt ------------------------------------------------------------------------ + + +def test_T284_the_receipt_carries_the_approvers_and_keeps_the_string(store, clock): + """§2.5, §11.3: `ctrlrun.receipt/v5`, and `approver` still says what 0.7.0 said.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + + receipt = _present(control, action, request_id) + + assert receipt.schema == "ctrlrun.receipt/v5" + assert RECEIPT_SCHEMA == "ctrlrun.receipt/v5" + assert receipt.approver == "mcp-operator:bob" + assert [who.agent for who in receipt.approvers] == ["human:bob"] + document = receipt.to_dict() + assert document["approvers"] == [ + { + "agent": "human:bob", + "user": "bob@example.com", + "issuer": "https://issuer.example", + "entitled": [], + "granted_at": document["approvers"][0]["granted_at"], + } + ] + assert "authority_grant_id" in document, "the v5 shape is frozen before item 5 fills it" + + +# --- T285, T286: G18 --------------------------------------------------------------------------- + + +def test_T285_self_approval_is_refused_on_the_principal_not_the_string(store, clock): + """G18: the strings differ and the principals are the same, which is the whole point.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(AGENT))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id, Principal(agent=AGENT.agent, user=AGENT.user)) + executor = _Executor() + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id, executor) + + assert refused.value.reason == IS_REQUESTER + assert executor.calls == 0 + assert store.get_approval(request_id).status == "granted" + + +def test_T286_a_different_principal_approves_and_the_action_runs(store, clock): + """G18's positive control: a guarantee that refused everything would pass T285 alone.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + executor = _Executor() + + receipt = _present(control, action, request_id, executor) + + assert executor.calls == 1 + assert str(receipt.result) == "committed" + + +# --- T287: a provider that raises, and one that declines --------------------------------------- + + +def test_T287_a_provider_that_raises_is_never_backfilled(store, clock): + """`v0.3 §3.2` at this door: a refused credential propagates rather than falling back. + + The raise is the point. There is no `context()` on the approval door to fall back to, so a + provider that rejects a credential must reach the surface that called it, which then grants + nothing, which the consume-side check then refuses for having no verified approver. + """ + identity = ApproverIdentity(_Recording(raises=IdentityError("rejected"))) + + with pytest.raises(IdentityError): + identity.resolve(IdentityContext(action="payments.refund", environment="production")) + + +def test_T287_a_declining_provider_produces_no_verified_approver(store, clock): + """A decline has nothing to fall back to here: there is no `context()` on this door.""" + identity = ApproverIdentity(_Recording(None)) + + assert identity.resolve(IdentityContext(action="payments.refund", environment="x")) is None + + +# --- T289: a static provider warns once -------------------------------------------------------- + + +def test_T289_a_static_approver_identity_warns_once_and_does_not_refuse(caplog): + """§2.3: a static provider answers with one name for every request. + + That is the deployment's choice to make and the record it produces is true, so this warns + rather than refuses; what is not true is that such a record distinguishes anybody. + """ + with caplog.at_level("WARNING"): + identity = ApproverIdentity(StaticIdentityProvider(agent="human:one")) + + assert identity.provider is not None + assert any("StaticIdentityProvider" in record.message for record in caplog.records) + + +# --- T290: the IdentityContext an approval resolution gets ------------------------------------- + + +def test_T290_the_context_names_the_stored_request_and_asserts_nothing(store, clock): + """§2.8: the action and environment of the action being approved, and no caller assertion.""" + provider = _Recording(APPROVER) + identity = ApproverIdentity(provider) + control = _control(store, clock, approver_identity=identity) + action = _action(control) + request_id = _requested(control, action) + record = store.get_approval(request_id) + + identity.resolve( + IdentityContext( + action=record.request.action.name, + environment=record.request.action.environment, + headers={"authorization": "Bearer x"}, + ) + ) + + context = provider.contexts[-1] + assert context.action == "payments.refund" + assert context.environment == control.environment + assert context.agent is None and context.user is None + + +# --- T291: the early return is gone ------------------------------------------------------------ + + +def test_T291_the_check_runs_with_no_precondition_provider_anywhere(store, clock): + """§2.4: the 0.6-shaped path is where every deployment lives, and where the check must run. + + `_recheck` returned immediately when no provider was named and the record carried no + fingerprint. A check added after that return is dead here, green, and invisible to a + mutation table. + """ + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + store.grant_approval(request_id, "cli:local") + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == UNVERIFIED + + +# --- T291b: the gate, all four rows ------------------------------------------------------------ + + +def test_T291b_a_denied_approval_still_denies(store, clock): + """§2.4.1 row 1: a human's no must not be reported as an approver problem.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + store.deny_approval(request_id, "cli:local") + + with pytest.raises(ActionDenied) as denied: + _present(control, action, request_id) + + assert denied.value.reason == "approval_denied" + types = [event.type for event in store.events() if event.approval_id == request_id] + assert EventType.APPROVAL_DENIED in types + receipts = [receipt for receipt in store.receipts() if receipt.action_id == action.action_id] + assert str(receipts[-1].result) == "denied" + + +def test_T291b_a_consumed_approval_still_reports_consumed(store, clock): + """§2.4.1 row 2: G2's replayed approval keeps its reason.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + _present(control, action, request_id) + + with pytest.raises(ApprovalMismatch) as replayed: + _present(control, action, request_id, key="refund:EU-43") + + assert replayed.value.reason == "consumed" + + +def test_T291b_a_moved_action_hash_still_reports_mismatch(store, clock): + """§2.4.1 row 3: G1 keeps its reason, which is `mismatch`.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + moved = _action(control, amount=999, payment_id="EU-42") + + with pytest.raises(ApprovalMismatch) as mismatched: + _present(control, moved, request_id) + + assert mismatched.value.reason == "mismatch" + + +def test_T291b_a_lapsed_grant_still_expires_with_its_event_and_its_write(store, clock): + """§2.4.1 row 4: the one a status-only gate fails: the lapse keeps its event and its write.""" + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + clock.advance(timedelta(hours=48)) + + with pytest.raises(ApprovalMismatch) as lapsed: + _present(control, action, request_id) + + assert lapsed.value.reason == "expired" + expired = [event for event in store.events() if event.type is EventType.APPROVAL_EXPIRED] + assert len(expired) == 1 + assert str(store.get_approval(request_id).status) == "expired" + + +# --- T295: the upgrade case -------------------------------------------------------------------- + + +def test_T295_an_approval_granted_before_the_provider_was_configured_is_refused(store, clock): + """§2.9: R1 working, and the sentence the changelog owes an operator.""" + before = _control(store, clock) + action = _action(before) + request_id = _requested(before, action) + before.store.grant_approval(request_id, "cli:local") + + after = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + + with pytest.raises(ApprovalMismatch) as refused: + _present(after, action, request_id) + + assert refused.value.reason == UNVERIFIED + + +# --- T296: observe mode ------------------------------------------------------------------------ + + +OBSERVE_POLICY = POLICY.replace("ctrlrun.policy/v1", "ctrlrun.policy/v3") + "mode: observe\n" + + +def test_T296_observe_mode_records_the_approver_reason_and_runs(store, clock): + """§4.1: and the deliberate consequence: a mismatch records its own reason, not a constant.""" + control = Control( + Policy.from_yaml(OBSERVE_POLICY), + store, + clock=clock, + approver_identity=ApproverIdentity(_Recording(AGENT)), + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + action = _action(control) + executor = _Executor() + + receipt = control.execute(action, executor, KEY) + + assert executor.calls == 1, "observe mode runs the action" + assert str(receipt.result) == "observed" + + +# --- T292, T293, T294 live beside the machinery they exercise ---------------------------------- + + +def test_T292_the_migration_keeps_every_row_and_adds_the_column(tmp_path, clock): + """§11.1's `0006_verified_approver`, forward-only, on a database with rows in it.""" + path = tmp_path / "state.db" + first = SQLiteStateStore(path, clock=clock) + control = _control(first, clock) + action = _action(control) + request_id = _requested(control, action) + first.store_approver = None + first.grant_approval(request_id, "cli:local") + first.close() + + reopened = SQLiteStateStore(path, clock=clock) + try: + record = reopened.get_approval(request_id) + assert record is not None + assert record.approver == "cli:local" + assert record.approvers == () + finally: + reopened.close() + + +def test_T293_a_v4_receipt_still_reads_and_a_v5_chain_verifies(store, clock): + """`v0.7 §6.11`: a receipt renders under its own schema, and the chain spans both.""" + from ctrlrun.receipt import Receipt + + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + _present(control, action, request_id) + + written = [receipt for receipt in store.receipts() if receipt.action_id == action.action_id] + document = written[-1].to_dict() + assert document["schema"] == "ctrlrun.receipt/v5" + + older = dict(document, schema="ctrlrun.receipt/v4") + older.pop("approvers") + older.pop("authority_grant_id") + parsed = Receipt.from_dict(older) + assert parsed.schema == "ctrlrun.receipt/v4" + assert parsed.approvers == () + assert "approvers" not in parsed.to_dict() + + +def test_T294_the_catalogue_is_v4_and_carries_G18(): + """§11.4: the version moves once, here, and G18 lands with it.""" + from ctrlrun.verify.guarantees import CATALOGUE, GUARANTEES + + assert CATALOGUE == "ctrlrun.guarantees/v4" + identifiers = [guarantee.id for guarantee in GUARANTEES] + assert "G18" in identifiers + assert identifiers == sorted(identifiers, key=lambda name: int(name[1:])) diff --git a/tests/test_attempt_cap.py b/tests/test_attempt_cap.py index f416931f..0f2ea11f 100644 --- a/tests/test_attempt_cap.py +++ b/tests/test_attempt_cap.py @@ -1626,7 +1626,7 @@ def _verify(tmp_path, document, *, only): def test_T252_G15_is_in_the_catalogue(): from ctrlrun.verify import guarantees as reg - assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" assert "G15" in reg.BY_ID assert "v0.1 §5.4" in reg.BY_ID["G15"].descends_from diff --git a/tests/test_clock_skew.py b/tests/test_clock_skew.py index f8e8b46c..c3c198db 100644 --- a/tests/test_clock_skew.py +++ b/tests/test_clock_skew.py @@ -582,7 +582,7 @@ def test_T219_G13_is_not_applicable_on_sqlite_with_its_sentence(tmp_path): def test_T219_the_catalogue_is_v3_and_G13_is_in_it(): - assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" assert "G13" in reg.BY_ID assert "v0.1 §5.3 E3" in reg.BY_ID["G13"].descends_from diff --git a/tests/test_demo.py b/tests/test_demo.py index f6ab3b8b..ab72c18c 100644 --- a/tests/test_demo.py +++ b/tests/test_demo.py @@ -102,6 +102,9 @@ # SPEC-v0.7 §6.11: `ctrlrun.receipt/v4`. Fingerprints, never the state they hash. "precondition_at_request", "precondition_at_recheck", + # SPEC-v0.8 §11.3: `ctrlrun.receipt/v5` adds the two. + "approvers", + "authority_grant_id", ) diff --git a/tests/test_idempotency.py b/tests/test_idempotency.py index 0fe37783..250e13be 100644 --- a/tests/test_idempotency.py +++ b/tests/test_idempotency.py @@ -677,7 +677,7 @@ def _g14(tmp_path, document: str = WITH_EFFECTS): def test_T239_G14_is_in_the_catalogue(): - assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" assert "G14" in reg.BY_ID assert reg.BY_ID["G14"].descends_from, "a guarantee names the tests it is the deployed form of" diff --git a/tests/test_mcp_operator.py b/tests/test_mcp_operator.py index 9034e8bf..efe14828 100644 --- a/tests/test_mcp_operator.py +++ b/tests/test_mcp_operator.py @@ -157,6 +157,45 @@ def _call(server, tool, arguments=None, *, credential=None, request_id=1): return json.loads(response.body), response.status +def test_T288_the_operator_server_records_the_principal_it_verified(server, control): + """SPEC-v0.8 §2.6 — the one shipped surface that can produce a verified approver. + + It has resolved a principal for every request since `SPEC-mcp-operator.md` shipped, and + then discarded it into the string `mcp-operator:`. What item 2 changed is that the + principal is recorded beside the string, so an approval granted here is consumable in a + deployment that checks (§2.7). + """ + from ctrlrun.approval import DEFAULT_APPROVAL_TTL + + action = _action(control) + request = control.approvals.request(action, DEFAULT_APPROVAL_TTL) + + document, status = _call( + server, "approve", {"request_id": request.request_id}, credential="alice" + ) + + assert status == 200, document + record = control.store.get_approval(request.request_id) + assert [(who.agent, who.user, who.issuer) for who in record.approvers] == [ + ("approver-app", "alice", "https://proxy.example/") + ] + assert record.approver == "mcp-operator:alice", "the string still says what it said" + + +def test_T288_a_denial_records_the_principal_too(server, control): + """§2.7's row for `deny_approval`: a denial is an act the evidence attributes.""" + from ctrlrun.approval import DEFAULT_APPROVAL_TTL + + action = _action(control) + request = control.approvals.request(action, DEFAULT_APPROVAL_TTL) + + document, status = _call(server, "deny", {"request_id": request.request_id}, credential="alice") + + assert status == 200, document + record = control.store.get_approval(request.request_id) + assert [who.agent for who in record.approvers] == ["approver-app"] + + def _rpc(server, method, params=None, *, request_id=1): body = {"jsonrpc": "2.0", "id": request_id, "method": method} if params is not None: diff --git a/tests/test_observe.py b/tests/test_observe.py index 7fb96bbe..6e80ff1d 100644 --- a/tests/test_observe.py +++ b/tests/test_observe.py @@ -206,7 +206,7 @@ def test_T82_the_observed_receipt_round_trips_through_json(store, clock): ) document = json.loads(receipt.to_json()) - assert document["schema"] == "ctrlrun.receipt/v4" + assert document["schema"] == "ctrlrun.receipt/v5" assert document["result"] == "observed" assert document["execution"] == "committed" assert document["would_have"]["decision"] == "deny" diff --git a/tests/test_preconditions.py b/tests/test_preconditions.py index 2c228402..b911862a 100644 --- a/tests/test_preconditions.py +++ b/tests/test_preconditions.py @@ -55,6 +55,7 @@ ) from ctrlrun.action import canonical_bytes from ctrlrun.approval import ApprovalStatus +from ctrlrun.migrations import HEAD from ctrlrun.receipt import RECEIPT_SCHEMA, EventType, ReceiptResult POLICY = """ @@ -304,11 +305,11 @@ def test_a_committed_receipt_records_that_the_world_was_checked(control, state_s receipt = present(control, action, request_id, world) - assert receipt.schema == RECEIPT_SCHEMA == "ctrlrun.receipt/v4" + assert receipt.schema == RECEIPT_SCHEMA == "ctrlrun.receipt/v5" assert receipt.precondition_at_request == fingerprint(AT_REQUEST) assert receipt.precondition_at_recheck == fingerprint(AT_REQUEST) document = receipt.to_dict() - assert document["schema"] == "ctrlrun.receipt/v4" + assert document["schema"] == "ctrlrun.receipt/v5" assert document["precondition_at_request"] == fingerprint(AT_REQUEST) assert document["precondition_at_recheck"] == fingerprint(AT_REQUEST) stored = state_store.get_approval(request_id) @@ -1411,12 +1412,15 @@ def test_T264_a_database_built_by_061_migrates_and_keeps_every_row(built_by_061) fake_clock = after_the_build facts = built.facts assert "0005_precondition_fingerprint" not in _applied(built), "0.6.1 knows 0005?" + assert "0006_verified_approver" not in _applied(built), "0.6.1 knows 0006?" columns_before = built.sql("SELECT * FROM approvals WHERE approval_id = ?", (facts["granted"],)) assert columns_before, "0.6.1 did not write the approval" store = built.open(fake_clock) - assert _applied(built)[-1] == "0005_precondition_fingerprint" + # HEAD, whatever it is: v0.8 item 2 adds `0006_verified_approver`, and what this test is + # about is that a database 0.6.1 built reaches HEAD with every row intact. + assert _applied(built)[-1] == HEAD for approval_id, status in ( (facts["consumed"], ApprovalStatus.CONSUMED), (facts["granted"], ApprovalStatus.GRANTED), @@ -1554,14 +1558,17 @@ def test_T265_a_chain_written_by_061_and_continued_by_07_verifies_end_to_end( store = _chain_continued_by_07(built, fake_clock) receipts = store.receipts() old = [receipt for receipt in receipts if receipt.schema == "ctrlrun.receipt/v3"] - new = [receipt for receipt in receipts if receipt.schema == "ctrlrun.receipt/v4"] + # The label this binary writes, which is `v5` since v0.8 item 2. What the test is about is + # unchanged: a chain written by 0.6.1 and continued by this binary verifies end to end, + # each receipt hashed by the rule its own version wrote. + new = [receipt for receipt in receipts if receipt.schema == "ctrlrun.receipt/v5"] assert len(old) == len(built.facts["receipts"]) >= 4 assert len(new) == 2 and len(receipts) == len(old) + len(new) for receipt in old: assert receipt.chain_hash() == receipt.hash, f"v3 seq {receipt.seq} rehashes differently" for receipt in new: document = json.loads(_stored_json(built, receipt.seq)) - assert document["schema"] == "ctrlrun.receipt/v4" + assert document["schema"] == "ctrlrun.receipt/v5" assert document["precondition_at_recheck"] == fingerprint(AT_REQUEST) report = verify_chain(store) @@ -2073,7 +2080,7 @@ def _g16(path, **kwargs): def test_T269_G16_is_in_the_v3_catalogue(): from ctrlrun.verify import guarantees as reg - assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" assert "G16" in reg.BY_ID assert reg.BY_ID["G16"].descends_from @@ -2323,6 +2330,7 @@ def test_every_schema_renders_under_its_own_label_and_key_set(): "ctrlrun.receipt/v2": 21, "ctrlrun.receipt/v3": 26, "ctrlrun.receipt/v4": 28, + "ctrlrun.receipt/v5": 30, "ctrlrun.receipt/v9": 26, "": 25, } @@ -2330,7 +2338,7 @@ def test_every_schema_renders_under_its_own_label_and_key_set(): document = replace(base, schema=label).to_dict() assert len(document) == count, (label, sorted(document)) assert document.get("schema") == (label or None) - if label != "ctrlrun.receipt/v4": + if label not in ("ctrlrun.receipt/v4", "ctrlrun.receipt/v5"): assert FABRICATED not in json.dumps(document), label assert replace(base, schema="ctrlrun.receipt/v1").to_dict()["principal"] == { "agent": "ops-agent", diff --git a/tests/test_protect.py b/tests/test_protect.py index b7802e51..f5708444 100644 --- a/tests/test_protect.py +++ b/tests/test_protect.py @@ -689,6 +689,11 @@ def read(customer_id: str) -> None: ... # only, and `null` here because nothing asked for a precondition. "precondition_at_request", "precondition_at_recheck", + # SPEC-v0.8 §11.3: `ctrlrun.receipt/v5` adds the two, and the whole shape is + # frozen before item 2 writes it, so `authority_grant_id` is present and null + # until item 5 fills it. + "approvers", + "authority_grant_id", } assert document["schema"] == RECEIPT_SCHEMA assert document["receipt_id"].startswith("ctr_") diff --git a/tests/test_verify.py b/tests/test_verify.py index 7a4fb0ec..991522d2 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -163,24 +163,27 @@ def test_T101_a_policy_with_no_approve_rule_makes_G1_and_G2_not_applicable(tmp_p # G16 is N/A for G1's reason: a precondition binds only where an approval is consumed # (SPEC-v0.7 §8.9), and nothing here requires one. - for gid in ("G1", "G2", "G16"): + # G18 joins them for the same reason, and it is a statement about this document: verify + # supplies the approver identity itself (SPEC-v0.8 §11.7), so what makes G18 inapplicable + # here is that nothing requires approval, never that nobody configured one. + for gid in ("G1", "G2", "G16", "G18"): assert results[gid].status is Status.NOT_APPLICABLE, gid assert results[gid].reason == reg.NO_APPROVE_RULE # The `else` branch: either one reported `pass` is the defect this test exists for. assert results[gid].status is not Status.PASS assert report.applicable == report.passed + report.failed - # G1, G2 and G16 for the missing approve band, G8 and G9 for the missing authority + # G1, G2, G16 and G18 for the missing approve band, G8 and G9 for the missing authority # section, G13, which is N/A on every SQLite run: SQLite has no clock of its own, and G15, # because this document names no `max_attempts` (SPEC-v0.7 §8.9). The rest are applicable, # G14 among them, and the count is over those. assert report.applicable == 9 - assert report.not_applicable == 7 + assert report.not_applicable == 8 text = report.to_text() - # The fraction is passes over applicable and never the catalogue size: with five N/As a - # thirteen-guarantee catalogue must not report thirteen over thirteen. + # The fraction is passes over applicable and never the catalogue size: with eight N/As a + # seventeen-guarantee catalogue must not report seventeen over seventeen. assert f"{len(reg.GUARANTEES)}/{len(reg.GUARANTEES)}" not in text assert f"{report.passed}/{report.applicable} declared guarantees pass." in text - assert "7 not applicable: G1, G2, G8, G9, G13, G15, G16." in text + assert "8 not applicable: G1, G2, G8, G9, G13, G15, G16, G18." in text def test_T101b_zero_applicable_guarantees_is_not_a_pass(tmp_path): @@ -767,16 +770,21 @@ def test_G11_is_applicable_even_where_every_action_is_denied(tmp_path): def test_the_catalogue_is_closed_and_ordered(): - """SPEC-v0.7 §9.4: `v3` is G1 to G16, and each id lands with its item. Ordered by number, + """SPEC-v0.8 §11.4: `v4` is G1 to G21, and each id lands with its item. Ordered by number, so an id that arrives before a lower one still sits where a reader looks for it, and - unreleased `main` carries a partial `v3` until item 6 asserts all sixteen.""" - assert reg.CATALOGUE == "ctrlrun.guarantees/v3" + unreleased `main` carries a partial `v4` until item 8 asserts all five. + + **No stub rows.** The upper bound is what v0.8 may reach; what is asserted about the middle + is that every id present is one of them and that none is missing from the order. A catalogue + holding an id whose check does not exist yet would report something before it could, which + is a false green (`v0.7 §9.4`'s D27).""" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" ids = [guarantee.id for guarantee in reg.GUARANTEES] assert ids[:11] == [f"G{n}" for n in range(1, 12)] - assert "G13" in ids and "G16" in ids + assert "G13" in ids and "G16" in ids and "G18" in ids assert ids == sorted(ids, key=lambda gid: int(gid[1:])), ids assert len(ids) == len(set(ids)) - assert set(ids) <= {f"G{n}" for n in range(1, 17)}, ids + assert set(ids) <= {f"G{n}" for n in range(1, 22)}, ids for guarantee in reg.GUARANTEES: assert guarantee.descends_from, f"{guarantee.id} names no acceptance test" @@ -844,17 +852,22 @@ def test_observe_mode_is_refused_before_any_scenario_runs(tmp_path): assert "observe" in str(refused.value) -def test_the_v1_payments_template_reports_eight_over_eight(): - """The definition of done, dogfooded rather than described (SPEC-v0.4 §4.1).""" +def test_the_v1_payments_template_reports_nine_over_nine(): + """The definition of done, dogfooded rather than described (SPEC-v0.4 §4.1). + + Nine and not eight since v0.8 item 2: G18 is graded here, because this document sends an + action to approval and verify supplies the approver identity it grades against (§11.7). + """ report = run(V1_PAYMENTS) assert report.exit_code == 0 - assert (report.passed, report.applicable, report.not_applicable) == (8, 8, 8) + assert (report.passed, report.applicable, report.not_applicable) == (9, 9, 8) text = report.to_text() - assert "8/8 declared guarantees pass." in text + assert "9/9 declared guarantees pass." in text # G13 is N/A on SQLite, which has no clock of its own; G14 and G15 join G3, G4 and G5 where # the effect template lives in the @protect decorator verify does not read, and where the - # document names no `max_attempts`. G16 is graded: verify brings its own provider (§8.9). + # document names no `max_attempts`. G16 and G18 are graded: verify brings its own provider + # for the first and its own approver identity for the second (§8.9, §11.7). assert "8 not applicable: G3, G4, G5, G8, G9, G13, G14, G15." in text assert "10/10" not in text diff --git a/tests/test_verify_action.py b/tests/test_verify_action.py index 0e3010af..d7415b9a 100644 --- a/tests/test_verify_action.py +++ b/tests/test_verify_action.py @@ -137,8 +137,8 @@ def test_T118_ci_asserts_the_two_shapes_the_specification_names(): steps = _workflow()["jobs"]["verify"]["steps"] script = "\n".join(step.get("run", "") for step in steps) - assert 'test "$AUTHORITY" = "verified 14/14"' in script - assert 'test "$TEMPLATES" = "verified 8/8"' in script + assert 'test "$AUTHORITY" = "verified 15/15"' in script + assert 'test "$TEMPLATES" = "verified 9/9"' in script assert 'test "$AUTHORITY_NA" = "2"' in script assert 'test "$TEMPLATES_NA" = "8"' in script @@ -152,12 +152,12 @@ def test_T118_the_two_configurations_really_do_report_those_shapes(): templates = run(V1_PAYMENTS) assert authority.badge is not None - assert authority.badge["message"] == "verified 14/14" + assert authority.badge["message"] == "verified 15/15" # 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 8/8" + assert templates.badge["message"] == "verified 9/9" assert templates.not_applicable == 8 @@ -200,7 +200,7 @@ def test_T119_the_denominator_is_applicable_and_never_the_catalogue_size(): assert badge is not None assert badge["message"] == f"verified {report.passed}/{report.applicable}" - assert report.applicable == 8 + assert report.applicable == 9 assert report.applicable < len(reg.GUARANTEES) assert f"/{len(reg.GUARANTEES)}" not in badge["message"] @@ -286,7 +286,7 @@ def test_T120_a_configuration_with_not_applicable_guarantees_still_writes_a_badg assert report.exit_code == 0 assert report.badge is not None - assert report.badge["message"] == "verified 8/8" + assert report.badge["message"] == "verified 9/9" def test_T120_a_failing_run_writes_a_red_badge_and_a_non_zero_exit(tmp_path, monkeypatch): diff --git a/tests/test_verify_authority.py b/tests/test_verify_authority.py index 2983d74b..a847356a 100644 --- a/tests/test_verify_authority.py +++ b/tests/test_verify_authority.py @@ -94,8 +94,10 @@ def test_T108_the_authority_example_exercises_G7_G8_and_G9(): assert report.exit_code == 0 # Graded on SQLite, where the one N/A is G13: SQLite has no clock of its own to diverge # from (SPEC-v0.7 §8.9). A Postgres --store-url grades that one too. - assert report.passed == 14 - assert report.applicable == 14 + # Fifteen since v0.8 item 2: G18 is graded wherever a document sends an action to + # approval, and verify supplies the approver identity it grades against (§11.7). + assert report.passed == 15 + assert report.applicable == 15 def test_T108_G8_asserts_the_denial_by_reason_and_not_by_type(tmp_path, monkeypatch): diff --git a/tests/test_verify_report.py b/tests/test_verify_report.py index cff56165..209ae168 100644 --- a/tests/test_verify_report.py +++ b/tests/test_verify_report.py @@ -110,8 +110,8 @@ def test_T113_the_summary_is_the_last_line_and_names_the_not_applicable_ids(tmp_ # template this document keeps in the @protect decorator, and G15 a `max_attempts` it does # not declare (SPEC-v0.7 §8.9). assert "8 not applicable: G3, G4, G5, G8, G9, G13, G14, G15." in last - # The fraction is passes over applicable. A report with eight N/As does not say 14/14. - assert "14/14" not in text + # The fraction is passes over applicable. A report with eight N/As does not say 17/17. + assert "17/17" not in text def test_T113_a_failing_report_names_the_subject_and_prints_the_counterexample( @@ -134,9 +134,12 @@ def test_T113_a_failing_report_names_the_subject_and_prints_the_counterexample( @pytest.mark.parametrize( ("document", "expected"), [ - (ALL_APPLICABLE, "12/12 declared guarantees pass. 4 not applicable"), - (WITH_NOT_APPLICABLE, "8/8 declared guarantees pass. 8 not applicable"), - (EMPTY, "0/0 declared guarantees pass. 16 not applicable"), + # SPEC-v0.8 item 2: G18 joins the catalogue. It is graded wherever the document sends + # an action to approval, which the first two of these do, and `N/A` for G1's reason + # where nothing does. So the first two gain a pass and the third gains an N/A. + (ALL_APPLICABLE, "13/13 declared guarantees pass. 4 not applicable"), + (WITH_NOT_APPLICABLE, "9/9 declared guarantees pass. 8 not applicable"), + (EMPTY, "0/0 declared guarantees pass. 17 not applicable"), ], ids=["passing", "some-na", "all-na"], ) @@ -186,7 +189,7 @@ def test_T114_the_document_matches_the_schema_field_for_field(tmp_path): assert set(document) == TOP_LEVEL assert document["schema"] == REPORT_SCHEMA == "ctrlrun.verify/v1" - assert document["catalogue"] == reg.CATALOGUE == "ctrlrun.guarantees/v3" + assert document["catalogue"] == reg.CATALOGUE == "ctrlrun.guarantees/v4" assert set(document["policy"]) == {"path", "sha256", "schema", "mode", "actions"} assert document["authority"] is None assert document["store"] == {"backend": "sqlite", "scratch": True} From fd6cd7a80053c5a35a41d482e814425383e6e489 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 14:03:07 +0530 Subject: [PATCH 2/6] Answer the item 2 review: a gate that skipped, and an observe row that did not exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two blocking, nine smaller, and the first is the worst defect this milestone has produced so far. The gate stood aside whenever THIS clock called the grant lapsed, because refusing there would cost the lapse its APPROVAL_EXPIRED event and the store's own lapse write. A skip is permanent and the store keeps its own clock: with this host running ahead, the gate stood aside, consume_approval_and_reserve consumed the grant by the store's clock, and the action ran with no approver check at all. The reviewer reproduced it as a self-approval committing under a twenty-minute skew, on the 0.6-shaped path §2.4 calls every deployment. That is v0.7 §12.5's divergence inverted from safe-and-untrue into fail-open. So the lapsed row is a deferral. Where the store disagrees and consumes a grant this clock called lapsed, §2.7 and §4.1 run on the record the presenting pass read and refuse; the grant is spent in that branch, which happens only where clocks already disagree, and spent-and-refused is the fail-closed direction. T291c is the reproduction and M3b and M3c are its mutations. The verdict is also computed once now, from one clock read, which §2.4.1 required and the first version did not do. And observe mode: §4.1 said it records approver_is_requester, _observe_take never called the check, so it recorded nothing. The section described something that did not exist, and T296 presented no approval at all, which made it a negative test against behaviour its own setup prevented. The check runs there now, _observe_secure records the mismatch's own reason where it recorded one constant for every mismatch, and T296 asserts both. That vocabulary change reaches refusals with nothing to do with v0.8, which the changelog says and which test_observe_mode_rechecks_records_and_runs_spending_no_grant now shows: a precondition refusal that used to read approval_mismatch reads precondition_changed. Smaller: a resumed leg's receipt dropped the approvers, and that leg is the only receipt an MCP multi round-trip or an ACS action ever gets; a corrupted approvers column threw a raw JSONDecodeError out of execute, and is now a named CTRLRunError, while the receipt reader keeps its never-raises contract for the opposite reason; verified_approver_now was a public name outside §11.1 and is now underscored; the static-provider warning compared a type name as a string, on the very shape §4.1 exists to refuse; §2.8 claimed a contract nothing calls, and now says item 3 wires it; T290, T292 and T293 asserted less than they claimed, and T292 and T293 now name where the real coverage lives. Fourteen mutations, all caught. Signed-off-by: arpan --- CHANGELOG.md | 7 ++ docs/SPEC-v0.8.md | 77 +++++++++--- src/ctrlrun/approval.py | 6 +- src/ctrlrun/control.py | 92 ++++++++++++-- src/ctrlrun/postgres.py | 6 +- src/ctrlrun/state.py | 31 +++-- src/ctrlrun/verify/scenarios.py | 2 +- tests/test_approver.py | 214 +++++++++++++++++++++++++++----- tests/test_preconditions.py | 8 +- 9 files changed, 370 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6e77501..79b5b6ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,13 @@ any change to one appears here. arrive through one of those three turns its approval path off by configuring this, which is the rule working rather than a defect, and §2.6's table is the thing to read before configuring. + **Observe mode now records a mismatch's own reason where it recorded one constant for all of + them.** `would_have.blocked_reason` said `approval_mismatch` for every `ApprovalMismatch`, so a + moved precondition and an approver who may not answer were one word in a report. Recording the + specific reason for the approver refusals alone would have left a vocabulary nobody can explain, + so every mismatch records its own. This reaches refusals that have nothing to do with v0.8, and + it is listed here rather than left for an operator to notice in a diff. + Needs `ctrlrun.receipt/v5`, which adds `approvers` and `authority_grant_id`, and migration `0006_verified_approver`. Every reader upgrades before any writer switches (`SPEC-v0.3.md` §12.2). diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 9e2f8844..1375a8e1 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -439,6 +439,24 @@ question being answered is "may this principal approve *this* action". `agent` a `None` and never the answering surface's assertion: `v0.3 §3.1` calls them a hint a provider may ignore, and a hint sourced from the caller of an approval command is the caller naming themselves. +### 2.8.1 What binds today, and what waits for item 3 + +**Item 2 does not call `ApproverIdentity.resolve`, and this section says so rather than reading as +though it did.** The one shipped surface that resolves an approver is the operator MCP server, and +it resolves through *its own* provider, built from `--principal-header` or `--identity-jwt`, with +the headers of the request being answered: that is what it has done since it shipped, and item 2 +changes only what it does with the answer. On `Control`, `ApproverIdentity` is the switch that +turns the consumption check on (§2.3), and its `provider` is what item 3 reads the roles claim +through. + +So the context above is the contract for a surface that resolves **through `ApproverIdentity`**, +and item 3 is where the operator server starts doing that, with `--approver-roles-claim` beside it. +Until then, `action` and `environment` on the server's own context name the *tool* being called, +which is `v0.3`'s shape for that server and not a promise this section made. + +A test that builds the context itself and then asserts the fields it just wrote proves nothing, and +§10.2's T290 was exactly that until an independent review said so. + ### 2.9 The upgrade note, stated because it will surprise somebody An approval granted at 0.7.0 and still pending in the store, presented after an `ApproverIdentity` @@ -1979,11 +1997,21 @@ building it confirmed: the only code that calls `grant_approval` outside a test operator server, `handle_inbound`, the scripted provider, the adapters, verify's own scenarios and `Control._withdraw`. None of them is `Control` deciding anything. -**The gate of §2.4.1 is `check_consumable` with its verdict's `record` tested and its `refusal` and -`expire` discarded.** It needed no new expiry logic, no second implementation of a frozen rule and -no new clock read: `control.py` already imports that function and already calls it twice. The -mutation table's M3 removes the gate and T291b's four rows go red together, which is what a gate -protecting four shipped reasons should do. +**The gate of §2.4.1 is `check_consumable`'s verdict, and `expire` is not discarded after all.** +The first version tested `verdict.record` and threw the rest away, which reads correctly and is +fail-open: `check_consumable` returns no record for a *lapsed* grant as well as for a denied, +consumed or hash-moved one, so on a host whose clock ran ahead of the store's the approver checks +stood aside, `consume_approval_and_reserve` then consumed the grant by the store's own clock, and +the action ran with **no approver check at all**. An independent review reproduced it: a +self-approval committing under a twenty-minute skew, which is `v0.7 §12.5`'s divergence inverted +from "safe and untrue" into fail-open, on the 0.6-shaped path §2.4 calls every deployment. + +So the lapsed row is a **deferral**, not a skip. Where the store disagrees and consumes a grant +this clock called lapsed, §2.7's and §4.1's checks run on the record the presenting pass read and +refuse. The grant is spent in that branch, which happens only where clocks already disagree, and +spent-and-refused is the fail-closed direction. T291c is the reproduction, and it goes red against +the skip. And the verdict is computed **once**, from one clock read, which §2.4.1 required and the +first version did not do: it read the clock twice, a tick apart. **The early return was exactly as dangerous as §2.4 said.** M6 restores it, every approver test in the file goes green, and only T291 fails: a check placed after that return is dead on the path @@ -2001,20 +2029,41 @@ for G12 as well. It is "the requester cannot approve" and not "self-approval is what is compared is the resolved principal on each side and "self" invites the reading that two different approver strings are two different people, which is the reading §4.1 exists to refuse. -**What the two version bumps moved in the suite, listed rather than absorbed.** Eighteen tests -outside this item's own file changed, and every one of them was a count or a key set that was true -of 0.7.0 and is not true now: four pin the receipt's exact JSON key set, which `v5` widens by two; -eleven pin verify counts, because G18 is graded wherever a document sends an action to approval, so -the shipped examples move from 14/14 to 15/15 and from 8/8 to 9/9; one pins the last migration by -name, and now asserts `HEAD`; and the remaining two pin the receipt schema label this binary -writes. **The verify counts are also pinned in `.github/workflows/ci.yml`**, which would have -turned the `verify` job red on a branch whose suite was entirely green, and which nothing in the -local gate would have caught. +**What the two version bumps moved in the suite, listed rather than absorbed.** Twelve test files +outside this item's own changed, and every edit in them was a count, a name or a key set that was +true of 0.7.0 and is not true now: the receipt's exact JSON key set, which `v5` widens by two; the +verify counts, because G18 is graded wherever a document sends an action to approval, so the +shipped examples move from 14/14 to 15/15 and from 8/8 to 9/9 and the catalogue pins move from +`v3` to `v4`; the last migration, pinned by name and now asserted as `HEAD`; and the receipt schema +label this binary writes. **The verify counts are also pinned in `.github/workflows/ci.yml`**, +which would have turned the `verify` job red on a branch whose suite was entirely green, and which +nothing in the local gate would have caught. **`_granting_principal` stayed package-internal and the operator server is its first caller.** That server has resolved a principal for every request since it shipped and then discarded it into `mcp-operator:`; item 2 is, on that surface, four lines that stop discarding it. +**Observe mode had to be implemented, not asserted.** §4.1's row said observe mode records +`approver_is_requester`, and `_observe_take` never called `_recheck`, so it recorded nothing: the +section described something that did not exist, and T296 presented no approval at all, which made +it a negative test against behaviour its own setup prevented. Both are fixed: the check runs on +that path, `_observe_secure` records the mismatch's own reason where it recorded one constant for +every mismatch, and T296 asserts the reason and the consequence. The vocabulary change is the one +§4.1 argued for, and it reaches refusals that have nothing to do with v0.8, which is why it is in +§11.1's table and in the changelog rather than left to a reader to notice. + +**A resumed leg's receipt is the only receipt some actions ever get.** `_resumed_context` +recovers the precondition fingerprints from the record and did not recover the approvers, so +§2.5's "carried onto the receipt" was false for exactly the MCP multi round-trip and ACS actions +that get one receipt (`SPEC-mcp-operator.md` §8.3). One line, and the review found it. + +**A corrupted `approvers` column is a `CTRLRunError` and a corrupted one on a receipt is not.** +The store read raises, named, because an approval is authority about to be spent and one bad row +must stop this action; `Receipt._approvers_of` drops a malformed entry instead, because a receipt +is evidence a reader walks past and one tampered row must not blind every reader at once +(`v0.7 §6.11`). The two rules look inconsistent and are the same rule applied to different +questions. + ### 14.3 Item 3: entitlement from the control registry ### 14.4 Item 4: M-of-N diff --git a/src/ctrlrun/approval.py b/src/ctrlrun/approval.py index 13ee67d3..e63f1a3c 100644 --- a/src/ctrlrun/approval.py +++ b/src/ctrlrun/approval.py @@ -28,7 +28,7 @@ CTRLRunError, InvalidArgument, ) -from .identity import IdentityContext, IdentityProvider +from .identity import IdentityContext, IdentityProvider, StaticIdentityProvider _LOG = logging.getLogger("ctrlrun") @@ -159,7 +159,7 @@ class ApproverIdentity: def __post_init__(self) -> None: if self.roles_claim is not None and not self.roles_claim.strip(): raise InvalidArgument("roles_claim must be a non-empty string or None") - if type(self.provider).__name__ == "StaticIdentityProvider": + if isinstance(self.provider, StaticIdentityProvider): # §2.3: a warning and not a refusal: a single-operator deployment where the shell # genuinely is the human is real, and the record it produces is true. What is not # true is that such a record distinguishes anybody, and an operator who has not @@ -201,7 +201,7 @@ def _granting_principal(principal: Principal, *, entitled: Iterable[str] = ()) - _GRANTING_PRINCIPAL.reset(token) -def verified_approver_now(now: datetime) -> VerifiedApprover | None: +def _verified_approver_now(now: datetime) -> VerifiedApprover | None: """The verified approver a store should record for a grant taken at `now`, if any. Read by the shipped stores inside `grant_approval` and `deny_approval`. A store that does diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index e7bd4481..c8c752b5 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -31,6 +31,7 @@ ApprovalRecord, ApprovalRequest, ApprovalStatus, + ApprovalVerdict, ApproverIdentity, LocalApprovalProvider, VerifiedApprover, @@ -373,19 +374,23 @@ class _Compared: per receipt, on a path that has just read the record. """ - __slots__ = ("approvers", "at_recheck", "at_request", "error") + __slots__ = ("approvers", "at_recheck", "at_request", "deferred_approver_check", "error") def __init__(self, at_request: str | None = None) -> None: self.at_request = at_request self.at_recheck: str | None = None self.error: str | None = None self.approvers: tuple[VerifiedApprover, ...] = () + #: SPEC-v0.8 §2.4.1: this clock called the grant lapsed, so the approver checks stood + #: aside; if the store disagrees and consumes it, they run after the store call. + self.deferred_approver_check = False def reset(self) -> None: self.at_request = None self.at_recheck = None self.error = None self.approvers = () + self.deferred_approver_check = False def data(self) -> dict[str, Any]: data: dict[str, Any] = { @@ -1176,7 +1181,15 @@ def _observe_secure( # A *presented* approval that does not authorize this action. §6.2 lists # `ApprovalMismatch` among the exceptions observe mode does not raise, so it is # recorded and the action runs; the approval is left unconsumed either way. - observation.block(BLOCKED_APPROVAL_MISMATCH) + # + # **The reason and not the constant, which is a behaviour change SPEC-v0.8 §4.1 + # argues for rather than a side effect of item 2.** This recorded + # `approval_mismatch` for every mismatch, so an operator reading an observe-mode + # report could not tell a moved precondition from an approver who may not answer. + # Recording the specific reason for the approver refusals alone would leave a + # vocabulary nobody can explain, so every mismatch now records its own reason. The + # values are the ones `_secure` raises, and §11.1's table lists them. + observation.block(mismatch.reason or BLOCKED_APPROVAL_MISMATCH) self._append( EventType.APPROVAL_INVALIDATED, action, @@ -1243,12 +1256,18 @@ def _observe_take( grant is spent. """ if approval_id is not None: + found = self._store.get_approval(approval_id) verdict = check_consumable( - self._store.get_approval(approval_id), + found, approval_id, action.action_hash, self._clock(), ) + # SPEC-v0.8 §4.1: observe mode records what enforce mode would have done, and + # enforce mode refuses a self-approval. This path never calls `_recheck`, so the + # check is made here too or the row §4.1 describes does not exist. Before the + # verdict's own refusal, on the same order `_recheck` uses. + self._check_approver(action, approval_id, found, compared, verdict) if verdict.refusal is not None: raise verdict.refusal # `as_approval()` and not a hand-built `Approval`: one construction, so observe @@ -1396,6 +1415,12 @@ def _resumed_context( # An approval consumed before this event carried the comparison, or by a path that # compared nothing: the record still says what it was requested with. compared.at_request = record.request.precondition_fingerprint + if record is not None: + # SPEC-v0.8 §2.5: who answered, recovered from the row for the same reason. **The + # resumed leg's receipt is the only receipt an MCP multi round-trip or an ACS action + # ever gets** (`SPEC-mcp-operator.md` §8.3), so without this the approvers reach the + # evidence on every action except the ones that get exactly one receipt. + compared.approvers = record.approvers return started, approval, compared def _outcome( @@ -1789,6 +1814,9 @@ def _secure( # (the attempt ceiling, §5.5) belongs before this line or after `_take`. self._recheck(action, approval_id, preconditions, compared) approval, reservation = self._take(action, approval_id, effect_key, lease) + # SPEC-v0.8 §2.4.1: the store disagreed with this clock about expiry and + # consumed the grant, so the checks that stood aside for the lapse run now. + self._check_approver_after_take(action, approval_id, compared) break except AmbiguousEffect as refused: # SPEC-v0.7 §3.6, before anything else: a store with its own clock re-measures @@ -2548,25 +2576,54 @@ def _recheck( record = self._store.get_approval(approval_id) stored = None if record is None else record.request.precondition_fingerprint compared.at_request = stored + # **One verdict, from one clock read**, reused by the approver gate below and by the + # precondition path's raise (SPEC-v0.8 §2.4.1). Two reads a tick apart could produce a + # gate that says "not lapsed" followed by a raise that says `expired`, which is the + # divergence `v0.7 §12.5` reversed. + verdict = check_consumable(record, approval_id, action.action_hash, self._clock()) # SPEC-v0.8 §2.4: **the early return is gone.** It returned here whenever no provider # was named and the record carried no fingerprint, which is every deployment that does # not use `v0.7 §6`, and an approver check added after it would have been dead on that # path, green, and invisible to a mutation table. - self._check_approver(action, approval_id, record, compared) + self._check_approver(action, approval_id, record, compared, verdict) if preconditions is None and stored is None: return - verdict = check_consumable(record, approval_id, action.action_hash, self._clock()) if verdict.refusal is not None: raise verdict.refusal assert record is not None # a verdict with no refusal carries its record self._compare(action, record, preconditions, compared) + def _check_approver_after_take( + self, action: Action, approval_id: str | None, compared: _Compared + ) -> None: + """The deferred approver check, run after the store disagreed about expiry (§2.4.1). + + **This is the fail-open hole an independent review found, closed.** The gate below skips + the approver checks where *this* clock considers the grant lapsed, because refusing here + would cost the lapse its `APPROVAL_EXPIRED` event and the store's own lapse write. But a + skip is permanent, and the store has its own clock: where this host runs ahead, the gate + skipped and `consume_approval_and_reserve` then consumed the grant happily, so the action + ran **with no approver check at all**. A self-approval committed under a twenty-minute + skew, which is `v0.7 §12.5`'s divergence inverted into fail-open. + + So the skip is a deferral and nothing more: where the store consumed a grant this clock + called lapsed, the checks run now, on the record read on the presenting pass, and refuse. + The grant is spent in that branch. That is the fail-closed direction and it costs a human + a second answer only on a deployment whose clocks already disagree, which `v0.7 §3` gives + an operator an event to find. + """ + if approval_id is None or not compared.deferred_approver_check: + return + record = self._store.get_approval(approval_id) + self._refuse_approver(action, approval_id, () if record is None else record.approvers) + def _check_approver( self, action: Action, approval_id: str, record: ApprovalRecord | None, compared: _Compared, + verdict: ApprovalVerdict, ) -> None: """Who answered, and whether they may have (SPEC-v0.8 §2.7, §4.1). @@ -2579,9 +2636,12 @@ def _check_approver( own lapse write**, because that write happens inside `_take` and a refusal raised here never reaches it. - The gate is `check_consumable`, the pure function `v0.1 §4.2` froze, with its `record` - tested and its `refusal` and `expire` discarded: no second implementation of a frozen - rule, no new clock read, and the store still decides expiry and may disagree. + The gate is the `check_consumable` verdict `_recheck` computed, the pure function + `v0.1 §4.2` froze: no second implementation of a frozen rule, no second clock read, and + the store still decides expiry and may disagree. + + **The lapsed row is a deferral and not a skip**, which is the difference between this and + the version an independent review broke: `_check_approver_after_take` says why. """ if record is not None: # Recorded whatever this deployment checks, so a receipt says who answered even @@ -2589,10 +2649,19 @@ def _check_approver( compared.approvers = record.approvers if self._approver_identity is None: return - verdict = check_consumable(record, approval_id, action.action_hash, self._clock()) if verdict.record is None: + # Denied, consumed or hash-moved: the store's reason wins and this check stands + # aside for good. Lapsed by this clock: stand aside *for now*, because the store may + # disagree, and a disagreement it resolves in favour of the grant must not be a way + # past §2.7. + compared.deferred_approver_check = verdict.expire return - approvers = verdict.record.approvers + self._refuse_approver(action, approval_id, verdict.record.approvers) + + def _refuse_approver( + self, action: Action, approval_id: str, approvers: tuple[VerifiedApprover, ...] + ) -> None: + """§2.7 and §4.1's two refusals, over whatever the row recorded.""" if not approvers: raise ApprovalMismatch( f"approval {approval_id} carries no verified approver, and this deployment " @@ -2936,7 +3005,6 @@ def _record( attempt: int = 1, observation: _Observation | None = None, compared: _Compared | None = None, - approvers: tuple[VerifiedApprover, ...] = (), ) -> Receipt: # SPEC-v0.3 §6.3 — one place turns a terminal outcome into an observed receipt, so # `result`, `execution` and `would_have` cannot disagree about the same action. The @@ -2977,7 +3045,7 @@ def _record( # SPEC-v0.8 §2.5: what §2 verified reaches the evidence, or the milestone records # nothing. Read from the row rather than from the `Approval`, which carries only the # string `v0.1 §4.1` froze. - approvers=approvers or (() if compared is None else compared.approvers), + approvers=() if compared is None else compared.approvers, ) # The store assigns `seq`, `prev_hash` and `hash` (SPEC-v0.6 §6.2, §6.3), so what goes # to the sinks and back to the caller is the **chained** receipt. Handing the unchained diff --git a/src/ctrlrun/postgres.py b/src/ctrlrun/postgres.py index c7887438..7706f798 100644 --- a/src/ctrlrun/postgres.py +++ b/src/ctrlrun/postgres.py @@ -46,9 +46,9 @@ ApprovalRecord, ApprovalRequest, ApprovalStatus, + _verified_approver_now, check_answerable, check_consumable, - verified_approver_now, ) from .effect import ( COMMITTED_EFFECT, @@ -1523,7 +1523,7 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: record = self._answerable(connection, approval_id, now) # SPEC-v0.8 §2.5: the verified approver the granting surface resolved, appended to # whatever the row already holds. - verified = verified_approver_now(now) + verified = _verified_approver_now(now) approvers = (*record.approvers, verified) if verified else record.approvers granted = replace( record, @@ -1567,7 +1567,7 @@ def deny_approval(self, approval_id: str, approver: str) -> None: self._use_schema(connection) try: record = self._answerable(connection, approval_id, now) - verified = verified_approver_now(now) + verified = _verified_approver_now(now) approvers = (*record.approvers, verified) if verified else record.approvers with connection.cursor() as cursor: cursor.execute( diff --git a/src/ctrlrun/state.py b/src/ctrlrun/state.py index 77f456e3..fe2c5fb0 100644 --- a/src/ctrlrun/state.py +++ b/src/ctrlrun/state.py @@ -38,9 +38,9 @@ ApprovalStatus, ApprovalStore, VerifiedApprover, + _verified_approver_now, check_answerable, check_consumable, - verified_approver_now, ) from .effect import ( COMMITTED_EFFECT, @@ -749,10 +749,26 @@ def _approvers_json(approvers: tuple[VerifiedApprover, ...]) -> str | None: def _approvers_from_json(text: str | None) -> tuple[VerifiedApprover, ...]: """What the column holds, or `()`. A column a store dropped reads as no approver at all, - which `Control` refuses at consumption rather than skipping (SPEC-v0.8 §2.5).""" + which `Control` refuses at consumption rather than skipping (SPEC-v0.8 §2.5). + + **A corrupted column is a `CTRLRunError` and not a `JSONDecodeError`.** This read sits under + `get_approval`, which sits under `_recheck`, which sits under `execute`: a raw decoding error + from a tampered row would reach an agent as an exception no caller catches and no receipt + records. Fail closed, named, and traceable to the row. + + It does **not** get `Receipt.from_dict`'s never-raises treatment, and the difference is + deliberate: a receipt is evidence a reader walks past, so one bad row must not blind every + reader, while an approval is authority about to be spent, so one bad row must stop this + action rather than be read as "no approver" (§2.5, `v0.7 §6.11`). + """ if not text: return () - return tuple(VerifiedApprover.from_dict(item) for item in json.loads(text)) + try: + return tuple(VerifiedApprover.from_dict(item) for item in json.loads(text)) + except (ValueError, TypeError, KeyError, AttributeError) as exc: + raise InvalidArgument( + f"the approvals row carries an unreadable 'approvers' column: {exc}" + ) from exc class InMemoryStateStore: @@ -898,7 +914,7 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: # SPEC-v0.8 §2.5: whatever the granting surface verified, or nothing where it # verified nobody. A store that did not read this records no approver, and that is # refused at consumption rather than skipped. - verified = verified_approver_now(now) + verified = _verified_approver_now(now) granted = replace( record, status=ApprovalStatus.GRANTED, @@ -913,7 +929,8 @@ def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) with self._lock: record = self._answerable(approval_id) - verified = verified_approver_now(self._clock()) + now = self._clock() + verified = _verified_approver_now(now) self._approvals[approval_id] = replace( record, status=ApprovalStatus.DENIED, @@ -1576,7 +1593,7 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: record = self._answerable(connection, approval_id, now) # SPEC-v0.8 §2.5: the verified approver, appended to whatever the row already # holds, inside the same `BEGIN IMMEDIATE` that serialises the status transition. - verified = verified_approver_now(now) + verified = _verified_approver_now(now) approvers = (*record.approvers, verified) if verified else record.approvers granted = replace( record, @@ -1609,7 +1626,7 @@ def deny_approval(self, approval_id: str, approver: str) -> None: connection.execute("BEGIN IMMEDIATE") try: record = self._answerable(connection, approval_id, now) - verified = verified_approver_now(now) + verified = _verified_approver_now(now) approvers = (*record.approvers, verified) if verified else record.approvers connection.execute( "UPDATE approvals SET status=?, approver=?, approvers=? WHERE approval_id=?", diff --git a/src/ctrlrun/verify/scenarios.py b/src/ctrlrun/verify/scenarios.py index 25953c1d..5031bf66 100644 --- a/src/ctrlrun/verify/scenarios.py +++ b/src/ctrlrun/verify/scenarios.py @@ -117,7 +117,7 @@ #: operator configured one, which is a fact about a constructor call and not about a document. #: #: Not `StaticIdentityProvider`: that one warns, by design (`v0.3 §3.3`), and a passing verify -#: run writes no kernel warning to stderr. +#: run writes no kernel warning to stderr. `_VerifyApproverProvider`, below, is what it uses. #: §3.6 — the base instant where no grant carries an `expires_at`. FALLBACK_T0: Final = datetime(2026, 1, 1, tzinfo=UTC) diff --git a/tests/test_approver.py b/tests/test_approver.py index e0592f16..871ba1e7 100644 --- a/tests/test_approver.py +++ b/tests/test_approver.py @@ -345,27 +345,28 @@ def test_T289_a_static_approver_identity_warns_once_and_does_not_refuse(caplog): # --- T290: the IdentityContext an approval resolution gets ------------------------------------- -def test_T290_the_context_names_the_stored_request_and_asserts_nothing(store, clock): - """§2.8: the action and environment of the action being approved, and no caller assertion.""" +def test_T290_the_approver_identity_is_a_switch_and_a_delegator_today(store, clock): + """§2.8.1: what item 2 actually wires, asserted instead of a context the test wrote itself. + + The first version of this built an `IdentityContext`, handed it to `identity.resolve`, and + asserted the fields it had just written: a tautology an independent review caught. Nothing + in the shipped tree calls `ApproverIdentity.resolve` yet. What item 2 wires is the switch, + and what §2.8's contract binds is a surface that resolves through it, which item 3 adds. + """ provider = _Recording(APPROVER) identity = ApproverIdentity(provider) control = _control(store, clock, approver_identity=identity) - action = _action(control) - request_id = _requested(control, action) - record = store.get_approval(request_id) - identity.resolve( - IdentityContext( - action=record.request.action.name, - environment=record.request.action.environment, - headers={"authorization": "Bearer x"}, - ) + assert control.approver_identity is identity, "the switch is readable, per §11.1" + assert provider.contexts == [], ( + "nothing in item 2 resolves through ApproverIdentity; §2.8.1 says so, and item 3 is " + "where that changes" ) - context = provider.contexts[-1] - assert context.action == "payments.refund" - assert context.environment == control.environment - assert context.agent is None and context.user is None + # And the delegation itself carries a provider's answer through without touching it. + given = IdentityContext(action="payments.refund", environment=control.environment) + assert identity.resolve(given) is APPROVER + assert provider.contexts == [given], "the context reaches the provider unaltered" # --- T291: the early return is gone ------------------------------------------------------------ @@ -454,6 +455,48 @@ def test_T291b_a_lapsed_grant_still_expires_with_its_event_and_its_write(store, assert str(store.get_approval(request_id).status) == "expired" +# --- T291c: the skew that turned the gate into a skip ------------------------------------------ + + +def test_T291c_a_clock_ahead_of_the_stores_does_not_skip_the_approver_check(store, clock): + """§2.4.1: the lapsed row is a deferral, not a skip, and this is why. + + An independent review broke the first version of the gate here. It stood aside whenever + *this* clock called the grant lapsed, because refusing on approver grounds would cost the + lapse its `APPROVAL_EXPIRED` event and the store's own lapse write. But a skip is permanent + and the store keeps its own clock: with this host running ahead, the gate stood aside, the + store consumed the grant happily, and the action ran **with no approver check at all**. The + reproduction was a self-approval committing under a twenty-minute skew. + + Here the store's clock stays still and the `Control`'s runs ahead of the expiry, which is + the same disagreement from the other side, and the approval is the requester's own. + """ + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(AGENT))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id, Principal(agent=AGENT.agent, user=AGENT.user)) + + ahead = _Clock() + ahead.now = clock.now + timedelta(hours=48) + skewed = Control( + Policy.from_yaml(POLICY), + store, + clock=ahead, + approver_identity=ApproverIdentity(_Recording(AGENT)), + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + executor = _Executor() + + with pytest.raises(ApprovalMismatch) as refused, with_approval(request_id): + skewed.execute(action, executor, KEY) + + assert refused.value.reason == IS_REQUESTER, ( + "the gate skipped the approver check because this clock called the grant lapsed, and " + "the store then consumed it: a self-approval ran" + ) + assert executor.calls == 0 + + # --- T295: the upgrade case -------------------------------------------------------------------- @@ -478,51 +521,155 @@ def test_T295_an_approval_granted_before_the_provider_was_configured_is_refused( OBSERVE_POLICY = POLICY.replace("ctrlrun.policy/v1", "ctrlrun.policy/v3") + "mode: observe\n" -def test_T296_observe_mode_records_the_approver_reason_and_runs(store, clock): - """§4.1: and the deliberate consequence: a mismatch records its own reason, not a constant.""" - control = Control( +def _observing(store, clock, identity): + return Control( Policy.from_yaml(OBSERVE_POLICY), store, clock=clock, - approver_identity=ApproverIdentity(_Recording(AGENT)), + approver_identity=identity, identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), ) - action = _action(control) + + +def test_T296_observe_mode_records_the_approver_reason_and_runs(store, clock): + """§4.1: what enforce mode would have done, recorded, with the action still running. + + **An approval is presented here, which the first version of this test did not do.** Without + one, `_observe_take` never reaches an approval at all, so the test asserted only that + observe mode runs the action, which is true of every observe-mode run and of a deployment + where this row is unimplemented. An independent review found it: a negative test against + behaviour the setup already prevented, in a window that was never opened. + """ + identity = ApproverIdentity(_Recording(AGENT)) + enforcing = _control(store, clock, approver_identity=identity) + action = _action(enforcing) + request_id = _requested(enforcing, action) + _grant_verified(store, request_id, Principal(agent=AGENT.agent, user=AGENT.user)) executor = _Executor() - receipt = control.execute(action, executor, KEY) + with with_approval(request_id): + receipt = _observing(store, clock, identity).execute(action, executor, KEY) - assert executor.calls == 1, "observe mode runs the action" + assert executor.calls == 1, "observe mode runs the action whatever it found" assert str(receipt.result) == "observed" + assert receipt.would_have is not None + assert receipt.would_have.blocked_reason == IS_REQUESTER + assert str(store.get_approval(request_id).status) == "granted", "no grant is spent" + + +def test_T296_every_other_mismatch_now_records_its_own_reason(store, clock): + """The deliberate consequence §4.1 names, asserted rather than left to a reader. + + Observe mode recorded the constant `approval_mismatch` for every `ApprovalMismatch`, so a + moved precondition and an approver who may not answer were one word in a report. Recording + the specific reason for the approver refusals alone would leave a vocabulary nobody can + explain, so every mismatch records its own reason now. Here: a hash that moved, which is + `G1`'s case and has nothing to do with v0.8. + """ + enforcing = _control(store, clock) + action = _action(enforcing) + request_id = _requested(enforcing, action) + store.grant_approval(request_id, "cli:local") + moved = _action(enforcing, amount=999, payment_id="EU-42") + executor = _Executor() + + with with_approval(request_id): + receipt = _observing(store, clock, None).execute(moved, executor, "refund:EU-99") + + assert executor.calls == 1 + assert receipt.would_have is not None + assert receipt.would_have.blocked_reason == "mismatch", ( + "the constant `approval_mismatch` is what this recorded before, for every mismatch" + ) + + +# --- T297: the resumed leg is the only receipt some actions get ------------------------------ + + +def test_T297_a_resumed_leg_carries_the_approvers_onto_its_receipt(store, clock): + """§2.5 and `SPEC-mcp-operator.md` §8.3, which is why this is not a nicety. + + A suspended action writes no receipt on the leg that suspended, so the resumed leg's is the + **only** receipt an MCP multi round-trip or an ACS action ever gets. `_resumed_context` + recovers the precondition fingerprints from the record and recovered nothing about who + approved, so "carried onto the receipt" was false for exactly those actions. An independent + review found it. + """ + from ctrlrun.errors import Suspended + + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + _grant_verified(store, request_id) + + def suspends(): + raise Suspended("continuation-EU-42") + + with pytest.raises(Suspended), with_approval(request_id): + control.execute(action, suspends, KEY) + + receipt = control.resume("continuation-EU-42", lambda: "refunded") + + assert str(receipt.result) == "committed" + assert receipt.approver == "mcp-operator:bob" + assert [who.agent for who in receipt.approvers] == ["human:bob"], ( + "the resumed leg is the only receipt this action gets, and it says who approved" + ) # --- T292, T293, T294 live beside the machinery they exercise ---------------------------------- -def test_T292_the_migration_keeps_every_row_and_adds_the_column(tmp_path, clock): - """§11.1's `0006_verified_approver`, forward-only, on a database with rows in it.""" +def test_T292_the_migration_is_at_head_and_the_column_round_trips(tmp_path, clock): + """`0006_verified_approver`, and what this test does **not** claim to cover. + + An independent review found the first version of this asserting nothing: it opened a store + with *this* binary, which applies `0006` at creation, reopened it, and called the survival + of a row evidence of a migration. No pre-`0006` database was ever built. + + The real upgrade coverage is `test_preconditions.py::test_T264`, which builds a database + with 0.6.1's own code and migrates it to `HEAD` with every row intact, on both backends. + What is left here is the half that belongs beside item 2: the column round-trips through a + real file-backed store, and the migration ledger says HEAD. + """ + from ctrlrun.migrations import HEAD + path = tmp_path / "state.db" first = SQLiteStateStore(path, clock=clock) control = _control(first, clock) action = _action(control) request_id = _requested(control, action) - first.store_approver = None - first.grant_approval(request_id, "cli:local") + _grant_verified(first, request_id) first.close() reopened = SQLiteStateStore(path, clock=clock) try: record = reopened.get_approval(request_id) assert record is not None - assert record.approver == "cli:local" - assert record.approvers == () + assert record.approver == "mcp-operator:bob" + assert [who.agent for who in record.approvers] == ["human:bob"] + applied = [ + row[0] + for row in reopened._connection().execute( + "SELECT migration_id FROM schema_version ORDER BY 1" + ) + ] + assert applied[-1] == HEAD + assert "0006_verified_approver" in applied finally: reopened.close() -def test_T293_a_v4_receipt_still_reads_and_a_v5_chain_verifies(store, clock): - """`v0.7 §6.11`: a receipt renders under its own schema, and the chain spans both.""" - from ctrlrun.receipt import Receipt +def test_T293_a_chain_spanning_two_receipt_schemas_verifies(store, clock): + """`v0.7 §6.11`: a receipt renders under its own schema, and the chain spans both. + + The first version round-tripped one relabelled document and never called `verify_chain`, + which an independent review called what it was. The cross-version chain over a *stored* v3 + and a continued v5 is `test_preconditions.py::test_T265`; this asserts the half item 2 adds, + that a v5 receipt's two new keys are read only from a v5 document and that a chain carrying + one verifies end to end. + """ + from ctrlrun.receipt import Receipt, verify_chain control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) action = _action(control) @@ -530,9 +677,14 @@ def test_T293_a_v4_receipt_still_reads_and_a_v5_chain_verifies(store, clock): _grant_verified(store, request_id) _present(control, action, request_id) + report = verify_chain(store) + assert report.ok, report.breaks + assert report.verified == len(store.receipts()) + written = [receipt for receipt in store.receipts() if receipt.action_id == action.action_id] document = written[-1].to_dict() assert document["schema"] == "ctrlrun.receipt/v5" + assert document["approvers"], "a v5 document carries what a v5 writer wrote" older = dict(document, schema="ctrlrun.receipt/v4") older.pop("approvers") diff --git a/tests/test_preconditions.py b/tests/test_preconditions.py index b911862a..11642eb5 100644 --- a/tests/test_preconditions.py +++ b/tests/test_preconditions.py @@ -988,7 +988,11 @@ def test_observe_mode_rechecks_records_and_runs_spending_no_grant(tmp_path, fake assert executor.calls == 1, "observe mode refused something" assert receipt.result is ReceiptResult.OBSERVED assert receipt.would_have is not None - assert receipt.would_have.blocked_reason == "approval_mismatch" + # SPEC-v0.8 §4.1: observe mode records the mismatch's **own** reason now, where it recorded + # the constant `approval_mismatch` for every one of them. This is a precondition refusal and + # has nothing to do with v0.8's approver; it is the clearest example of what that change + # reaches, which is why the changelog lists it rather than leaving it to a diff. + assert receipt.would_have.blocked_reason == CHANGED data = invalidated(store, request_id)[0].data assert data["reason"] == CHANGED assert data["precondition_at_recheck"] == fingerprint(MOVED) @@ -2077,7 +2081,7 @@ def _g16(path, **kwargs): return report, next(result for result in report.guarantees if result.id == "G16") -def test_T269_G16_is_in_the_v3_catalogue(): +def test_T269_G16_is_in_the_catalogue(): from ctrlrun.verify import guarantees as reg assert reg.CATALOGUE == "ctrlrun.guarantees/v4" From 9bea8c5d597e03c65eafc904c69850c52570a4a4 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 14:37:01 +0530 Subject: [PATCH 3/6] Answer round two: the deferral strands a reservation, so the lapsed row is checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three blocking, two of them defects introduced by round one's fixes, which is the shape CONTRIBUTING asks a second pass for. The deferral closed the fail-open and moved the damage. consume_approval_and _reserve does both halves of v0.1 §4.2 A4 in one transaction, so a refusal after it left a consumed grant, an effect key RESERVED with a live lease and nothing to release it, no EFFECT_RESERVED and no APPROVAL_CONSUMED event, and a receipt whose error said the approval was left granted when it was not. The key lapses into AMBIGUOUS: a human resolving an action the kernel itself refused and which provably never ran, which is the shape _spend_unneeded_approval already records a review finding for. There is no clean cleanup either: fail_effect requires EXECUTING so RESERVED to FAILED does not exist, and mark_ambiguous would assert unknown about an action known not to have run. So the lapsed row is checked before _take like every other row. verdict.expire is read rather than discarded. The cost is one narrow case, a grant that is both lapsed and approver-refused keeps no APPROVAL_EXPIRED and no lapse write, and what it buys is that nothing is written by a refusal stays literally true, which neither the skip nor the deferral could say. §2.4.2 records all three attempts because the next reader will reach for one of the two wrong ones. §2.4.1 still specified the design the review rejected. Only the retrospective §14.2 had been updated, so a reader implementing from the normative section, or a third-party Control, would have built the skip. §2.4, §2.4.1, §2.4.2 and §12 now describe what the code does. And the observe-mode vocabulary change had a consumer nobody had looked at. would_have.blocked_reason is a closed set because ctrlrun stats buckets counts on it, and receipt.py says a bucketed count over a string nobody constrained is a report that quietly stops adding up. Recording each mismatch's own reason made that true: an observe-mode approval refusal landed in no bucket, would_have_been_blocked went from 1 to 0, and nothing in the suite went red because the only assertions on that field use an effect-state reason. BLOCKED_BY_STATE grew with the change and T296b asserts the count, driven with no ApproverIdentity anywhere because the receipt that stopped counting was a plain hash mismatch. Smaller: T291c now asserts the grant is still granted and nothing reserved, so it can tell a refusal before _take from one after; mismatch.reason or the constant was a subsumed guard, since reason is a required keyword; §2.4 says what observe mode's refusal costs a reservation; and the deferral's dead flag, its post-take read and the InvalidArgument that could escape through it are all gone with it. Fourteen mutations, all caught. 3645 passed. Signed-off-by: arpan --- docs/SPEC-v0.8.md | 203 +++++++++++++++++++++-------------------- src/ctrlrun/control.py | 67 +++++--------- src/ctrlrun/receipt.py | 30 +++++- tests/test_approver.py | 72 ++++++++++++++- 4 files changed, 227 insertions(+), 145 deletions(-) diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 1375a8e1..374a940a 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -238,9 +238,9 @@ before the store call that consumes the approval (`v0.7 §6.2`). The approver ch and the order is normative: 1. `get_approval(approval_id)`, the read `_recheck` already performs. -2. **A gate, and it is load-bearing: the approver checks apply only to a record that is `granted` - and that this clock does not already consider lapsed.** Everything else is left to the store - exactly as today (§2.4.1). +2. **A gate, and it is load-bearing: the approver checks stand aside for a record the store will + refuse for its own reason** (denied, consumed, hash-moved, pending, unknown), and run on every + other, the lapsed row included. §2.4.1 and §2.4.2 carry the table and the argument. 3. **The approver checks of §2.7, §3.6, §4.1 and §4.2**, in that order, each raising `ApprovalMismatch` with its own reason. 4. The precondition comparison of `v0.7 §6.2`, unchanged, and last before the store call because it @@ -255,10 +255,32 @@ use `v0.7 §6`. Adding the approver checks after that return would leave them de path, green, and mutation-invisible. With an `ApproverIdentity` configured, the read and the approver checks run on **every** presenting pass under `APPROVE`. -**`check_consumable` stays where it is.** `Control` does not apply it on the no-fingerprint path -and v0.8 does not move it: whose clock decides an approval's expiry is `v0.1 §4.2 A3`'s question, -the answer is the store's, and `v0.7 §12.5` reversed a change that got that wrong. The approver -checks are decidable from the row alone and need no clock. +**The gate is `check_consumable`'s own verdict, computed once from one clock read**, and reused by +`_recheck`'s existing precondition raise. Not a second implementation of a frozen rule, and not a +second clock read: two reads a tick apart could produce a gate that says "not lapsed" followed by a +raise that says `expired`, which is the divergence `v0.7 §12.5` reversed. + +**The expiry *decision* stays the store's.** `Control` does not refuse for expiry and does not +write a lapse: a lapsed grant whose approver is fine falls through to `_take` untouched, which is +`v0.1 §4.2 A3`'s answer. What this clock is used for is knowing which rows the gate stands aside +for, and §2.4.2 is the argument for the one row where that distinction is load-bearing. + +**Nothing is written by a refusal, and §2.4.2 is why that sentence survived.** Not the approval, +which stays granted, on `v0.6 §7.2`'s precedent: the action is refused, the human's yes is not +spent on a question it did not answer, and the approval still expires. Not the effect, because +nothing is reserved. The refusal produces an `APPROVAL_INVALIDATED` event and a `BLOCKED` receipt, +exactly as a precondition refusal does. **Every refusal in §2 and §4 is raised before `_take` for +exactly this reason**: a check that refuses after the store call cannot say this, and the design +that tried was rejected for it. + +**Observe mode reaches these checks through a different path**, `_observe_take`, which does not +call `_recheck`, so §4.1's row is implemented there too. Two consequences, both stated because +neither is obvious. An `ApprovalMismatch` raised there returns before `reserve_effect`, so an +observed action refused on approver grounds takes no reservation: not new behaviour, but a new +member of a class every observe-mode `ApprovalMismatch` was already in, and one case short of +`_observe_take`'s own argument for reserving. And `_observe_secure` records the mismatch's **own +reason** where it recorded one constant for every mismatch, which §4.1 argues and which §11.1's +table and `BLOCKED_BY_STATE` both had to grow for. ### 2.4.1 Why the gate exists, and what it costs to omit it @@ -271,53 +293,54 @@ only inside `_take`: | A human **denied** it | `ActionDenied(approval_denied)`, with `APPROVAL_DENIED`, `ACTION_DENIED` and a `DENIED` receipt | `ApprovalMismatch(approver_unverified)`, with `APPROVAL_INVALIDATED` and a `BLOCKED` receipt: a human's no stops appearing in the evidence as a no | | Already **consumed** (a replayed approval, G2) | `consumed` | `approver_unverified` | | The action **hash moved** (G1) | `mismatch`, which is `HASH_MISMATCH`'s value | `approver_unverified` | -| `granted` and **past its expiry** | `expired`, with `APPROVAL_EXPIRED` appended and the row moved to `expired` by the store's own lapse write | `approver_unverified`, **no `APPROVAL_EXPIRED` event, and no lapse write at all** | - -All four carry no `VerifiedApprover`, because nothing granted them under an approver identity, so -§2.7's second row would fire first and swallow the real reason. Two of them are shipped guarantees. -The gate keeps `check_consumable`'s ordering authoritative, which is what `v0.1 §4.2` froze it for. - -**The fourth row is why the gate is not just a status test**, and it is the subtler half. -`check_consumable` tests expiry **after** status, so a lapsed grant is still `granted` and would -pass a status-only gate. Two things then disappear rather than one: `_secure` appends -`APPROVAL_EXPIRED` only when the reason it caught is `expired`, and the lapse **write** happens -only inside `_take`, where `check_consumable` returns `expire=True` and the store performs it. A -refusal raised before `_take` never reaches either, so the row stays `granted` for ever and the -lapse leaves no event. §2.4's "nothing is written by a refusal" is about the approver refusals and -was never about this write, which `v0.1 §4.2 A3` requires. - -**One verdict, from one clock read.** The gate is `check_consumable(record, approval_id, -action_hash, self._clock())` with its `record` tested and its `refusal` and `expire` **discarded**: -the pure function `v0.1 §4.2` froze already decides exactly this, in exactly this order, and -`control.py` already imports and calls it. So there is no second implementation and no new clock -read. The verdict is computed **once** and reused by `_recheck`'s existing precondition-path raise: -two reads a tick apart could produce a gate that says "not lapsed" followed by a raise that says -`expired`, which is the divergence shape `v0.7 §12.5` reversed. - -**And the clock in this gate only ever defers.** Where this `Control`'s clock says the grant has -lapsed, the approver checks are **skipped** and `_take` reports the lapse exactly as 0.7.0 does, -with its write and its event. The clock is never used to *refuse*: the store still decides, and may -disagree, which is `v0.1 §4.2 A3`'s answer and the one `v0.7 §12.5` restored after a change got it -wrong. So "the approver checks need no clock" stays true of the checks; what needs one is knowing -when to stand aside. - -T291b is the test, all four rows. - -**Why the approver checks go before the precondition fetch.** A refused approval must not cost a -provider call (`v0.7 §6.6` argues the same for a refused verdict), and an approver who was never -entitled does not become entitled because a balance is unchanged. - -**Nothing is written by a refusal.** Not the approval, which stays granted, on `v0.6 §7.2`'s -precedent: the action is refused, the human's yes is not spent on a question it did not answer, and -the approval still expires. Not the effect, because nothing is reserved. The refusal produces an -`APPROVAL_INVALIDATED` event and a `BLOCKED` receipt, exactly as a precondition refusal does. +| **Pending**, or no such approval | its own status | `approver_unverified` | + +So the gate stands aside for all four, the store's reason wins, and two shipped guarantees keep +theirs. + +### 2.4.2 The lapsed row, which took three attempts + +**A grant that is `granted`, whose hash matches, and that is past its expiry by *this* clock is the +one row where `check_consumable` refuses a record the approver checks can still read.** It is +`verdict.expire`. What to do with it is the hardest decision in §2, it was got wrong twice, and +both wrong answers are recorded here because the next reader will reach for one of them. + +**The first answer was to skip it**, on the reasoning that refusing on approver grounds would cost +the lapse its `APPROVAL_EXPIRED` event and the store's own lapse write, both of which happen inside +`_take`. That is fail-open. The store keeps its own clock, so on a host running ahead the checks +stood aside, `consume_approval_and_reserve` consumed the grant by the store's clock, and the action +ran **with no approver check at all**: a self-approval committing under a twenty-minute skew. It is +`v0.7 §12.5`'s divergence inverted from "safe and untrue" into fail-open, on the path §2.4 calls +every deployment that does not use `v0.7 §6`. + +**The second answer was to defer it past `_take`** and refuse once the store had disagreed and +consumed. That closes the hole and moves the damage: `consume_approval_and_reserve` does both +halves of `v0.1 §4.2 A4` in one transaction, so the refusal then left a **consumed grant**, an +effect key **`RESERVED` with a live lease and nothing to release it**, no `EFFECT_RESERVED` and no +`APPROVAL_CONSUMED` event, and a receipt whose error said the approval was left granted when it was +not. The key then lapses into `AMBIGUOUS`, which is a human resolving an action the kernel itself +refused and which provably never ran. `_spend_unneeded_approval`'s docstring records an earlier +review finding the same shape: *an ambiguity manufactured by the permissive decision path*. And +there is no clean cleanup: `fail_effect` requires `EXECUTING`, so `RESERVED → FAILED` does not +exist, and `mark_ambiguous` would assert "unknown" about an action known not to have run. + +**The answer is to check it, before `_take`, like the others.** The gate stands aside only for the +four rows above; the lapsed row is checked. + +| The row | What it reports | What it costs | +|---|---|---| +| Lapsed, approver **fine** | `expired`, with `APPROVAL_EXPIRED` and the store's own lapse write | nothing: the check passes, `_take` decides, and `v0.1 §4.2 A3` keeps the expiry decision with the store | +| Lapsed, approver **refused** | the approver reason | that row keeps no `APPROVAL_EXPIRED` and no lapse write | -**Observe mode reaches these checks through a different path**, `_observe_take`, which does not -call `_recheck`. §4.1's observe-mode row is implemented there, and §11.1 records the consequence: -`_observe_secure` records the fixed constant `approval_mismatch` for every `ApprovalMismatch` -today, so reporting a specific reason for the approver refusals changes what observe mode records -for **every** mismatch. That is a behaviour change, it is listed in §11.1's reason table, and the -changelog carries it under "stricter than 0.7.0". +The cost is the second row and it is the cheapest of the three answers. The grant is unusable +either way: `check_consumable` refuses it at every later presentation, and it expires for real. +What is bought is that the lapsed row cannot be a way past §2.7 on a host whose clock runs ahead, +and that **nothing is written by a refusal** stays literally true, which neither of the other two +answers could say. + +T291b covers the four rows and the lapsed-with-a-good-approver row; T291c is the skew reproduction +and asserts the grant is still granted and nothing reserved; the lapsed-with-a-bad-approver row has +its own test saying what it gives up. ### 2.5 How a verified approver reaches the row, without a new store method @@ -1873,7 +1896,7 @@ G16's `PRECONDITION_NOTE` is the precedent for how this was handled last time, a | Situation | Outcome | |---|---| -| `ApproverIdentity` configured, row carries no verified approver | `ApprovalMismatch(approver_unverified)`; nothing reserved; approval left granted | +| `ApproverIdentity` configured, row carries no verified approver | `ApprovalMismatch(approver_unverified)`; nothing reserved; approval left granted. True literally, because every refusal in §2 and §4 is raised before `_take` (§2.4.2) | | Approver provider raises | Refused at the answering surface; never backfilled from the calling code (`v0.3 §3.2`) | | Approver provider declines | Refused at the answering surface; no approval granted | | Roles claim absent, or carried by a provider that carries no claims | `approver_unentitled`, with a warning naming the claim and the control | @@ -1882,6 +1905,9 @@ G16's `PRECONDITION_NOTE` is the precedent for how this was handled last time, a | `roles_claim` unset and a role is required | `approver_unentitled` | | Several controls with roles, one unsatisfied | `approver_unentitled`, naming that control | | Approver equals requester | `approver_is_requester` | +| A grant that is lapsed by this clock **and** refused on approver grounds | The approver reason, and that row keeps no `APPROVAL_EXPIRED` and no lapse write (§2.4.2) | +| A grant lapsed by this clock whose approver is fine | `expired`, with its event and the store's own lapse write: the check passes and `_take` decides | +| A corrupted `approvers` column | `InvalidArgument` from the store read, named and traceable to the row; a corrupted one on a *receipt* is dropped instead, because a receipt is evidence a reader walks past (§14.2) | | Fewer than `approvals_required` distinct approvers | Record stays `pending`; consumption refused with `pending` | | `approvals_required > 1` with no `ApproverIdentity` | Action denied with `approvals_unverifiable`, naming the action and the key (§4.2) | | Store that does not record several approvers, N > 1 | Never reaches N; never behaves as N = 1 | @@ -1997,51 +2023,30 @@ building it confirmed: the only code that calls `grant_approval` outside a test operator server, `handle_inbound`, the scripted provider, the adapters, verify's own scenarios and `Control._withdraw`. None of them is `Control` deciding anything. -**The gate of §2.4.1 is `check_consumable`'s verdict, and `expire` is not discarded after all.** -The first version tested `verdict.record` and threw the rest away, which reads correctly and is -fail-open: `check_consumable` returns no record for a *lapsed* grant as well as for a denied, -consumed or hash-moved one, so on a host whose clock ran ahead of the store's the approver checks -stood aside, `consume_approval_and_reserve` then consumed the grant by the store's own clock, and -the action ran with **no approver check at all**. An independent review reproduced it: a -self-approval committing under a twenty-minute skew, which is `v0.7 §12.5`'s divergence inverted -from "safe and untrue" into fail-open, on the 0.6-shaped path §2.4 calls every deployment. - -So the lapsed row is a **deferral**, not a skip. Where the store disagrees and consumes a grant -this clock called lapsed, §2.7's and §4.1's checks run on the record the presenting pass read and -refuse. The grant is spent in that branch, which happens only where clocks already disagree, and -spent-and-refused is the fail-closed direction. T291c is the reproduction, and it goes red against -the skip. And the verdict is computed **once**, from one clock read, which §2.4.1 required and the -first version did not do: it read the clock twice, a tick apart. - -**The early return was exactly as dangerous as §2.4 said.** M6 restores it, every approver test in -the file goes green, and only T291 fails: a check placed after that return is dead on the path -every 0.6-shaped deployment takes, and nothing else in the suite notices. - -**One test file covering one store is one store covered.** The first draft of `test_approver.py` -used the in-memory store alone, and the mutation table caught it: blanking the verified approver in -the **SQLite** write path left all twenty tests green, because none of them had ever executed that -path. The fixture now runs every test on in-memory, SQLite and Postgres, which is `v0.6 §2`'s -argument for the store conformance suite applied to a test file, and M7a and M7b are two rows -rather than one. - -**G18's title is 28 characters because the report table is 32 wide**, which v0.7 had to discover -for G12 as well. It is "the requester cannot approve" and not "self-approval is refused", because -what is compared is the resolved principal on each side and "self" invites the reading that two -different approver strings are two different people, which is the reading §4.1 exists to refuse. - -**What the two version bumps moved in the suite, listed rather than absorbed.** Twelve test files -outside this item's own changed, and every edit in them was a count, a name or a key set that was -true of 0.7.0 and is not true now: the receipt's exact JSON key set, which `v5` widens by two; the -verify counts, because G18 is graded wherever a document sends an action to approval, so the -shipped examples move from 14/14 to 15/15 and from 8/8 to 9/9 and the catalogue pins move from -`v3` to `v4`; the last migration, pinned by name and now asserted as `HEAD`; and the receipt schema -label this binary writes. **The verify counts are also pinned in `.github/workflows/ci.yml`**, -which would have turned the `verify` job red on a branch whose suite was entirely green, and which -nothing in the local gate would have caught. - -**`_granting_principal` stayed package-internal and the operator server is its first caller.** That -server has resolved a principal for every request since it shipped and then discarded it into -`mcp-operator:`; item 2 is, on that surface, four lines that stop discarding it. +**The gate took three attempts and §2.4.2 records all three**, because the next reader will reach +for one of the two that were wrong. Skipping the lapsed row is fail-open under clock skew: a +self-approval committed, reproduced by the review. Deferring it past `_take` closes that and +strands a reservation the kernel cannot release, which lapses into an `AMBIGUOUS` record a human +must resolve for an action the kernel itself refused; `fail_effect` requires `EXECUTING`, so there +is no `RESERVED → FAILED` to clean it up with, and `mark_ambiguous` would assert "unknown" about an +action known not to have run. Checking it before `_take` like every other row costs one thing, that +a grant which is both lapsed and approver-refused keeps no `APPROVAL_EXPIRED` and no lapse write, +and buys the sentence the other two answers could not say: **nothing is written by a refusal**. + +**Two of those three were found by review rather than by the suite**, and the second was a defect +introduced by the fix for the first, which is the shape `CONTRIBUTING.md` asks a second review pass +for. The tests that exist now are the ones that would have caught them: T291c reproduces the skew +**and asserts the grant is still granted and nothing reserved**, which the first version of that +test did not, so it could not have told a refusal before `_take` from one after. + +**The observe-mode vocabulary change had a consumer nobody had looked at.** `would_have. +blocked_reason` is a closed set because `ctrlrun stats` buckets counts on it, and `receipt.py` says +so in as many words: *a bucketed count over a string nobody constrained is a report that quietly +stops adding up*. Recording each mismatch's own reason made exactly that true, measured: an +observe-mode approval refusal landed in no bucket, `would_have_been_blocked` went from 1 to 0, and +the command whose whole job is "what changes if you turn enforcement on" under-reported it. The set +grew with the change, T296b asserts the count, and the receipt that stopped counting was a plain +hash mismatch with nothing to do with v0.8. **Observe mode had to be implemented, not asserted.** §4.1's row said observe mode records `approver_is_requester`, and `_observe_take` never called `_recheck`, so it recorded nothing: the diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index c8c752b5..57d1dcc5 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -88,7 +88,6 @@ ) from .receipt import ( BLOCKED_AMBIGUOUS, - BLOCKED_APPROVAL_MISMATCH, BLOCKED_APPROVAL_REQUIRED, BLOCKED_ATTEMPT_CEILING, BLOCKED_DUPLICATE, @@ -374,23 +373,19 @@ class _Compared: per receipt, on a path that has just read the record. """ - __slots__ = ("approvers", "at_recheck", "at_request", "deferred_approver_check", "error") + __slots__ = ("approvers", "at_recheck", "at_request", "error") def __init__(self, at_request: str | None = None) -> None: self.at_request = at_request self.at_recheck: str | None = None self.error: str | None = None self.approvers: tuple[VerifiedApprover, ...] = () - #: SPEC-v0.8 §2.4.1: this clock called the grant lapsed, so the approver checks stood - #: aside; if the store disagrees and consumes it, they run after the store call. - self.deferred_approver_check = False def reset(self) -> None: self.at_request = None self.at_recheck = None self.error = None self.approvers = () - self.deferred_approver_check = False def data(self) -> dict[str, Any]: data: dict[str, Any] = { @@ -1189,7 +1184,7 @@ def _observe_secure( # Recording the specific reason for the approver refusals alone would leave a # vocabulary nobody can explain, so every mismatch now records its own reason. The # values are the ones `_secure` raises, and §11.1's table lists them. - observation.block(mismatch.reason or BLOCKED_APPROVAL_MISMATCH) + observation.block(mismatch.reason) self._append( EventType.APPROVAL_INVALIDATED, action, @@ -1814,9 +1809,6 @@ def _secure( # (the attempt ceiling, §5.5) belongs before this line or after `_take`. self._recheck(action, approval_id, preconditions, compared) approval, reservation = self._take(action, approval_id, effect_key, lease) - # SPEC-v0.8 §2.4.1: the store disagreed with this clock about expiry and - # consumed the grant, so the checks that stood aside for the lapse run now. - self._check_approver_after_take(action, approval_id, compared) break except AmbiguousEffect as refused: # SPEC-v0.7 §3.6, before anything else: a store with its own clock re-measures @@ -2593,30 +2585,6 @@ def _recheck( assert record is not None # a verdict with no refusal carries its record self._compare(action, record, preconditions, compared) - def _check_approver_after_take( - self, action: Action, approval_id: str | None, compared: _Compared - ) -> None: - """The deferred approver check, run after the store disagreed about expiry (§2.4.1). - - **This is the fail-open hole an independent review found, closed.** The gate below skips - the approver checks where *this* clock considers the grant lapsed, because refusing here - would cost the lapse its `APPROVAL_EXPIRED` event and the store's own lapse write. But a - skip is permanent, and the store has its own clock: where this host runs ahead, the gate - skipped and `consume_approval_and_reserve` then consumed the grant happily, so the action - ran **with no approver check at all**. A self-approval committed under a twenty-minute - skew, which is `v0.7 §12.5`'s divergence inverted into fail-open. - - So the skip is a deferral and nothing more: where the store consumed a grant this clock - called lapsed, the checks run now, on the record read on the presenting pass, and refuse. - The grant is spent in that branch. That is the fail-closed direction and it costs a human - a second answer only on a deployment whose clocks already disagree, which `v0.7 §3` gives - an operator an event to find. - """ - if approval_id is None or not compared.deferred_approver_check: - return - record = self._store.get_approval(approval_id) - self._refuse_approver(action, approval_id, () if record is None else record.approvers) - def _check_approver( self, action: Action, @@ -2640,8 +2608,10 @@ def _check_approver( `v0.1 §4.2` froze: no second implementation of a frozen rule, no second clock read, and the store still decides expiry and may disagree. - **The lapsed row is a deferral and not a skip**, which is the difference between this and - the version an independent review broke: `_check_approver_after_take` says why. + **The lapsed row is checked and not skipped**, which is the difference between this and + the version an independent review broke twice: first by skipping it, which was fail-open + under clock skew, and then by deferring it past `_take`, which closed that and left a + consumed grant and a reservation nothing releases. The comment below carries the cost. """ if record is not None: # Recorded whatever this deployment checks, so a receipt says who answered even @@ -2649,14 +2619,25 @@ def _check_approver( compared.approvers = record.approvers if self._approver_identity is None: return - if verdict.record is None: - # Denied, consumed or hash-moved: the store's reason wins and this check stands - # aside for good. Lapsed by this clock: stand aside *for now*, because the store may - # disagree, and a disagreement it resolves in favour of the grant must not be a way - # past §2.7. - compared.deferred_approver_check = verdict.expire + if verdict.record is None and not verdict.expire: + # Denied, consumed, hash-moved, pending, unknown: the store's reason wins and this + # check stands aside, because an ungated refusal would report an approver problem + # for a human's no, for `G1`'s moved hash and for `G2`'s replayed approval. return - self._refuse_approver(action, approval_id, verdict.record.approvers) + # **`verdict.expire` is the lapsed row, and it is checked rather than skipped.** It means + # granted, hash matching, and past its expiry by *this* clock, which is the one case where + # `check_consumable` refuses a record the approver checks can still read. Skipping it was + # fail-open: the store keeps its own clock, so where this host ran ahead the checks stood + # aside and `consume_approval_and_reserve` then consumed the grant happily, and a + # self-approval committed under a twenty-minute skew. + # + # What it costs to check it here instead: a grant that is **both** lapsed and refused on + # approver grounds reports the approver reason rather than `expired`, so that row keeps no + # `APPROVAL_EXPIRED` event and no lapse write. The grant is unusable either way, + # `check_consumable` refuses it at every later presentation, and a lapsed grant whose + # approver is fine still reports `expired` with its event and the store's own write, + # because the check passes and `_take` decides. §2.4.1 carries the table. + self._refuse_approver(action, approval_id, record.approvers if record is not None else ()) def _refuse_approver( self, action: Action, approval_id: str, approvers: tuple[VerifiedApprover, ...] diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index ab131365..becc6d45 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -109,7 +109,34 @@ #: 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 +#: SPEC-v0.8 §4.1 — observe mode records a mismatch's **own** reason now, where it recorded +#: `BLOCKED_APPROVAL_MISMATCH` for every one of them, so the closed vocabulary above grows by the +#: reasons an `ApprovalMismatch` actually carries. They are the values `check_consumable` and +#: `Control` already raise, listed here because §6.4 buckets counts on this set. +#: +#: **Widening the set is not decoration: without it the change would have been a silent +#: under-count.** An independent review measured it. An observe-mode approval refusal, including a +#: plain hash mismatch that has nothing to do with v0.8, landed in no bucket at all, so +#: `would_have_been_blocked` went from 1 to 0 and `ctrlrun stats` under-reported exactly what it +#: exists to report. The comment above says a bucketed count over a string nobody constrained is a +#: report that quietly stops adding up; this is that, and the fix is to constrain the string. +BLOCKED_APPROVAL_REASONS: Final = frozenset( + { + "mismatch", + "consumed", + "expired", + "pending", + "denied", + "unknown", + "precondition_changed", + "precondition_missing", + "precondition_unavailable", + "approver_unverified", + "approver_is_requester", + } +) + +#: The ones 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( { @@ -118,6 +145,7 @@ BLOCKED_IN_PROGRESS, BLOCKED_AMBIGUOUS, BLOCKED_ATTEMPT_CEILING, + *BLOCKED_APPROVAL_REASONS, } ) diff --git a/tests/test_approver.py b/tests/test_approver.py index 871ba1e7..7c898024 100644 --- a/tests/test_approver.py +++ b/tests/test_approver.py @@ -438,8 +438,14 @@ def test_T291b_a_moved_action_hash_still_reports_mismatch(store, clock): assert mismatched.value.reason == "mismatch" -def test_T291b_a_lapsed_grant_still_expires_with_its_event_and_its_write(store, clock): - """§2.4.1 row 4: the one a status-only gate fails: the lapse keeps its event and its write.""" +def test_T291b_a_lapsed_grant_whose_approver_is_fine_still_expires(store, clock): + """§2.4.1 row 4: the lapse keeps its reason, its event and the store's own write. + + This is the row that makes the gate more than a status test, and it holds because the + approver check **passes** here: a lapsed grant with a good approver falls through to + `_take`, which is where `v0.1 §4.2 A3` says the expiry decision and its write belong. The + row where the approver check would refuse is the test below. + """ control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) action = _action(control) request_id = _requested(control, action) @@ -455,6 +461,29 @@ def test_T291b_a_lapsed_grant_still_expires_with_its_event_and_its_write(store, assert str(store.get_approval(request_id).status) == "expired" +def test_T291b_a_lapsed_grant_with_a_bad_approver_reports_the_approver(store, clock): + """§2.4.1's fifth row, which is what checking the lapsed row costs. + + A grant that is **both** lapsed and refused on approver grounds reports the approver reason, + so that row keeps no `APPROVAL_EXPIRED` event and no lapse write. The grant is unusable + either way and `check_consumable` refuses it at every later presentation; what is bought is + that the lapsed row cannot be a way past §2.7 on a host whose clock runs ahead of the + store's, which is T291c. + """ + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + store.grant_approval(request_id, "cli:local") + clock.advance(timedelta(hours=48)) + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == UNVERIFIED + assert [event.type for event in store.events()].count(EventType.APPROVAL_EXPIRED) == 0 + assert str(store.get_approval(request_id).status) == "granted" + + # --- T291c: the skew that turned the gate into a skip ------------------------------------------ @@ -495,6 +524,13 @@ def test_T291c_a_clock_ahead_of_the_stores_does_not_skip_the_approver_check(stor "the store then consumed it: a self-approval ran" ) assert executor.calls == 0 + # **And the refusal costs nothing**, which the first fix for this could not say: it deferred + # the check past `_take`, so the grant was consumed and the effect key left RESERVED with a + # live lease and nothing to release it, which lapses into an `AMBIGUOUS` record a human must + # resolve for an action the kernel itself refused. The review found that too. + record = store.get_approval(request_id) + assert str(record.status) == "granted", "the human's yes was spent on a refusal" + assert store.get_effect(KEY) is None, "a refused action left a reservation behind" # --- T295: the upgrade case -------------------------------------------------------------------- @@ -583,6 +619,38 @@ def test_T296_every_other_mismatch_now_records_its_own_reason(store, clock): ) +# --- T296b: the report still adds up ----------------------------------------------------------- + + +def test_T296b_an_observed_approval_refusal_is_still_counted_by_stats(store, clock): + """§4.1's vocabulary change, against the bucket `ctrlrun stats` counts on (`v0.3 §6.4`). + + An independent review measured what this change did before the bucket was widened: an + observe-mode approval refusal landed in no bucket at all, so `would_have_been_blocked` went + from 1 to 0 and the command whose whole job is "what changes if you turn enforcement on" + silently under-reported it. `receipt.py` already said why that must not happen: a bucketed + count over a string nobody constrained is a report that quietly stops adding up. + + Driven with **no `ApproverIdentity` anywhere**, because the receipt that stopped counting was + a plain hash mismatch with nothing to do with v0.8. + """ + from ctrlrun.reporting import stats_document + + enforcing = _control(store, clock) + action = _action(enforcing) + request_id = _requested(enforcing, action) + store.grant_approval(request_id, "cli:local") + moved = _action(enforcing, amount=999, payment_id="EU-42") + + with with_approval(request_id): + _observing(store, clock, None).execute(moved, _Executor(), "refund:EU-99") + + document = stats_document(list(store.receipts()), mode="observe", boundary=None) + + assert document["would_have_been_blocked"] == 1 + assert document["blocked_by_reason"] == {"mismatch": 1} + + # --- T297: the resumed leg is the only receipt some actions get ------------------------------ From 154a86f108fc304947282197323532b411fc1e5c Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 15:06:14 +0530 Subject: [PATCH 4/6] Answer round three: a human's no was counted nowhere, and five paragraphs I dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing blocking survived. Five smaller findings, and one of them is older than this milestone. check_consumable catches a denied record one branch before the generic status branch, so what observe mode records for it is approval_denied and not denied. The set this item introduced carried "denied", which nothing on that path can produce, and missed "approval_denied", which is recorded: an observe-mode run where a human refused was counted in no bucket at all, which is close to the most important thing such a report can say. That predates v0.8. It is fixed here because this is the commit that writes the set and argues at length for closing exactly this, and shipping it wrong would have made the argument false on its own terms. T296b's sibling drives a human's denial through observe mode. The gate's fourth row, pending or no such approval, had no test, and §2.4.2 claimed T291b covered it. It does now, both values, and pending is the reason §2.7's fifth row and all of item 4's M-of-N depend on: a gate narrowed to exclude it would have gone unnoticed and taken M-of-N's refusal with it. record.approvers if record is not None else () was a subsumed guard: control reaches it only where verdict.record carries the record or verdict.expire is set, which check_consumable can only reach after record is None has returned. An assertion says so instead of an else branch pretending a case exists. "Every refusal in §2 and §4 is raised before _take" was wider than the mechanism: §2.7's fifth row is the store's own pending, raised inside _take, which is where it belongs. And five paragraphs of §14.2 went missing in the round-three rewrite, including the only record of the .github/workflows/ci.yml verify-count pin, which that paragraph itself describes as something nothing in the local gate would have caught. Restored, with the bucket finding beside them. Fourteen mutations, all caught. 3651 passed. Signed-off-by: arpan --- docs/SPEC-v0.8.md | 46 +++++++++++++++++++++++++++++++++++++++--- src/ctrlrun/receipt.py | 14 +++++++++++-- tests/test_approver.py | 44 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 5 deletions(-) diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 374a940a..4baee8c5 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -269,9 +269,11 @@ for, and §2.4.2 is the argument for the one row where that distinction is load- which stays granted, on `v0.6 §7.2`'s precedent: the action is refused, the human's yes is not spent on a question it did not answer, and the approval still expires. Not the effect, because nothing is reserved. The refusal produces an `APPROVAL_INVALIDATED` event and a `BLOCKED` receipt, -exactly as a precondition refusal does. **Every refusal in §2 and §4 is raised before `_take` for -exactly this reason**: a check that refuses after the store call cannot say this, and the design -that tried was rejected for it. +exactly as a precondition refusal does. **Every refusal the approver checks make is raised before +`_take` for exactly this reason**: a check that refuses after the store call cannot say this, and +the design that tried was rejected for it (§2.4.2). Not every refusal in §2 and §4 is one of +theirs: §2.7's fifth row, fewer than `approvals_required` distinct approvers, is the store's own +`pending`, raised inside `_take`, which is where it belongs. **Observe mode reaches these checks through a different path**, `_observe_take`, which does not call `_recheck`, so §4.1's row is implemented there too. Two consequences, both stated because @@ -2048,6 +2050,44 @@ the command whose whole job is "what changes if you turn enforcement on" under-r grew with the change, T296b asserts the count, and the receipt that stopped counting was a plain hash mismatch with nothing to do with v0.8. +**The early return was exactly as dangerous as §2.4 said.** M6 restores it, every approver test in +the file goes green, and only T291 fails: a check placed after that return is dead on the path +every 0.6-shaped deployment takes, and nothing else in the suite notices. + +**One test file covering one store is one store covered.** The first draft of `test_approver.py` +used the in-memory store alone, and the mutation table caught it: blanking the verified approver in +the **SQLite** write path left every test green, because none of them had ever executed that path. +The fixture now runs every test on in-memory, SQLite and Postgres, which is `v0.6 §2`'s argument +for the store conformance suite applied to a test file, and M7a and M7b are two rows rather than +one. + +**G18's title is 28 characters because the report table is 32 wide**, which v0.7 had to discover +for G12 as well. It is "the requester cannot approve" and not "self-approval is refused", because +what is compared is the resolved principal on each side and "self" invites the reading that two +different approver strings are two different people, which is the reading §4.1 exists to refuse. + +**What the two version bumps moved in the suite, listed rather than absorbed.** Twelve test files +outside this item's own changed, and every edit in them was a count, a name or a key set that was +true of 0.7.0 and is not now: the receipt's exact JSON key set, which `v5` widens by two; the +verify counts, because G18 is graded wherever a document sends an action to approval, so the +shipped examples move from 14/14 to 15/15 and from 8/8 to 9/9 and the catalogue pins move from `v3` +to `v4`; the last migration, pinned by name and now asserted as `HEAD`; and the receipt schema +label this binary writes. **The verify counts are also pinned in `.github/workflows/ci.yml`**, +which would have turned the `verify` job red on a branch whose suite was entirely green, and which +nothing in the local gate would have caught. + +**`_granting_principal` stayed package-internal and the operator server is its first caller.** That +server has resolved a principal for every request since it shipped and then discarded it into +`mcp-operator:`; item 2 is, on that surface, four lines that stop discarding it. + +**And the bucket this item widened was already missing a human's no.** `check_consumable` catches a +denied record one branch before the generic status branch, so the reason recorded for it is +`approval_denied` and not `denied`: the set as first written carried a value nothing can produce +and missed the one that is, and an observe-mode run where a human refused was counted nowhere. +That predates v0.8. It is fixed here because this is the commit that writes the set and argues at +length for closing exactly this, and shipping it wrong would have made the argument false on its +own terms. + **Observe mode had to be implemented, not asserted.** §4.1's row said observe mode records `approver_is_requester`, and `_observe_take` never called `_recheck`, so it recorded nothing: the section described something that did not exist, and T296 presented no approval at all, which made diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index becc6d45..28edab65 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -26,6 +26,7 @@ # `approval.py` imports `action`, `errors` and `identity` and nothing else, so this is # downward (ARCHITECTURE §6): a receipt records what an approval verified, and the record # type it records is that module's. +from .approval import APPROVAL_DENIED as APPROVAL_DENIED_REASON from .approval import VerifiedApprover from .errors import CTRLRunError, InvalidArgument from .policy import Decision @@ -111,7 +112,8 @@ #: SPEC-v0.8 §4.1 — observe mode records a mismatch's **own** reason now, where it recorded #: `BLOCKED_APPROVAL_MISMATCH` for every one of them, so the closed vocabulary above grows by the -#: reasons an `ApprovalMismatch` actually carries. They are the values `check_consumable` and +#: reasons an approval refusal actually carries, whether it is raised as an `ApprovalMismatch` +#: or, for a human's no, as an `ActionDenied`. They are the values `check_consumable` and #: `Control` already raise, listed here because §6.4 buckets counts on this set. #: #: **Widening the set is not decoration: without it the change would have been a silent @@ -120,14 +122,22 @@ #: `would_have_been_blocked` went from 1 to 0 and `ctrlrun stats` under-reported exactly what it #: exists to report. The comment above says a bucketed count over a string nobody constrained is a #: report that quietly stops adding up; this is that, and the fix is to constrain the string. +#: **`approval_denied` and not `denied`, and the difference is a report that was already wrong.** +#: `check_consumable` catches a denied record one branch before the generic status branch and +#: raises `ActionDenied(reason=APPROVAL_DENIED)`, so `"denied"`, which is `str(ApprovalStatus. +#: DENIED)`, is a value nothing on this path can carry, while `"approval_denied"` is recorded by +#: observe mode's `ActionDenied` handler and was in no bucket at all. An observe-mode run where a +#: **human said no** was therefore counted nowhere, which is close to the most important thing +#: such a report can say. That predates v0.8 and is fixed here, because this is the commit that +#: writes the set and argues for closing it. BLOCKED_APPROVAL_REASONS: Final = frozenset( { "mismatch", "consumed", "expired", "pending", - "denied", "unknown", + APPROVAL_DENIED_REASON, "precondition_changed", "precondition_missing", "precondition_unavailable", diff --git a/tests/test_approver.py b/tests/test_approver.py index 7c898024..1a56f91b 100644 --- a/tests/test_approver.py +++ b/tests/test_approver.py @@ -484,6 +484,25 @@ def test_T291b_a_lapsed_grant_with_a_bad_approver_reports_the_approver(store, cl assert str(store.get_approval(request_id).status) == "granted" +def test_T291b_a_pending_or_unknown_approval_keeps_its_own_reason(store, clock): + """§2.4.1's fourth row, which had no test until an independent review said so. + + `pending` is the reason §2.7's fifth row and all of item 4's M-of-N depend on, so a gate + narrowed to exclude it would go unnoticed and take M-of-N's refusal with it. + """ + control = _control(store, clock, approver_identity=ApproverIdentity(_Recording(APPROVER))) + action = _action(control) + request_id = _requested(control, action) + + with pytest.raises(ApprovalMismatch) as pending: + _present(control, action, request_id) + assert pending.value.reason == "pending" + + with pytest.raises(ApprovalMismatch) as unknown: + _present(control, action, "apr_" + "0" * 32) + assert unknown.value.reason == "unknown" + + # --- T291c: the skew that turned the gate into a skip ------------------------------------------ @@ -651,6 +670,31 @@ def test_T296b_an_observed_approval_refusal_is_still_counted_by_stats(store, clo assert document["blocked_by_reason"] == {"mismatch": 1} +def test_T296b_an_observed_denial_by_a_human_is_counted_too(store, clock): + """The half of the bucket that was wrong before v0.8 touched it (§4.1, `v0.3 §6.4`). + + `check_consumable` catches a denied record one branch before the generic status branch and + raises `ActionDenied(reason="approval_denied")`, so the set carried `"denied"`, which nothing + on this path can produce, and missed `"approval_denied"`, which observe mode records. An + observe-mode run where a **human said no** was counted nowhere. An independent review found + it while checking the set this item introduced. + """ + from ctrlrun.reporting import stats_document + + enforcing = _control(store, clock) + action = _action(enforcing) + request_id = _requested(enforcing, action) + store.deny_approval(request_id, "cli:local") + + with with_approval(request_id): + _observing(store, clock, None).execute(action, _Executor(), KEY) + + document = stats_document(list(store.receipts()), mode="observe", boundary=None) + + assert document["would_have_been_blocked"] == 1 + assert document["blocked_by_reason"] == {"approval_denied": 1} + + # --- T297: the resumed leg is the only receipt some actions get ------------------------------ From 73fb387570084a478add18f6450db0bf4c815fc3 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 16:14:58 +0530 Subject: [PATCH 5/6] Answer CodeRabbit: str is a Sequence, and four inaccuracies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all real, and one is a data-integrity bug rather than a wording fix. VerifiedApprover.from_dict did tuple(document.get("entitled") or ()), and str is a Sequence: tuple("abc") is ("a","b","c"). A corrupted column holding a bare string was therefore accepted as an approver entitled for three controls that do not exist, silently, and _approvers_from_json's promise to raise on a corrupted row was false for exactly the shape a corruption most easily takes. entitled is now validated before conversion, as a list of non-empty strings, read as object because the declared type is what a caller promises and this value comes from a JSON column. T283b drives the four shapes. That is the same hazard SPEC-v0.8 §3.4 states for the roles claim, which the independent review made me write into the spec and which then landed in the code one field over. The other four are accuracy. The changelog said verify grades sixteen guarantees; the catalogue is G1 to G16 plus G18, which is seventeen. §2.4 said nothing is written by a refusal and then described the event and receipt it writes: it now says no approval is mutated and no effect reserved, and that evidence is written, because a refusal nobody can find afterwards is not a refusal this project ships. §2.4.1 merged the pending and unknown rows, which carry different reasons and have separate tests. And §14.2 listed Control._withdraw among grant_approval's callers when it calls deny_approval, which §2.6's table already gives its own row. 3654 passed. Signed-off-by: arpan --- CHANGELOG.md | 2 +- docs/SPEC-v0.8.md | 19 ++++++++++++------- src/ctrlrun/approval.py | 21 ++++++++++++++++++++- tests/test_approver.py | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b5b6ea..a47a2cc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ any change to one appears here. before the provider was configured**, including one granted through a surface that cannot resolve, and including one held by a store that ignores the column. - **`ctrlrun verify` grades sixteen guarantees now**, G18 among them under + **`ctrlrun verify` grades seventeen guarantees now**, G18 among them under `ctrlrun.guarantees/v4`: an approval granted by the principal that requested the action is refused, compared on the resolved principal and never on the string. Verify supplies the approver identity it grades against, so what it reports is the kernel's refusal and never whether an diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 4baee8c5..24285e01 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -265,10 +265,12 @@ write a lapse: a lapsed grant whose approver is fine falls through to `_take` un `v0.1 §4.2 A3`'s answer. What this clock is used for is knowing which rows the gate stands aside for, and §2.4.2 is the argument for the one row where that distinction is load-bearing. -**Nothing is written by a refusal, and §2.4.2 is why that sentence survived.** Not the approval, -which stays granted, on `v0.6 §7.2`'s precedent: the action is refused, the human's yes is not -spent on a question it did not answer, and the approval still expires. Not the effect, because -nothing is reserved. The refusal produces an `APPROVAL_INVALIDATED` event and a `BLOCKED` receipt, +**A refusal mutates no approval and reserves no effect, and §2.4.2 is why that sentence +survived.** Not the approval, which stays granted, on `v0.6 §7.2`'s precedent: the action is +refused, the human's yes is not spent on a question it did not answer, and the approval still +expires. Not the effect, because nothing is reserved. **Evidence is written**, and the distinction +is the point: a refusal appends `APPROVAL_INVALIDATED` and writes a `BLOCKED` receipt, because a +refusal nobody can find afterwards is not a refusal this project ships. The refusal produces an `APPROVAL_INVALIDATED` event and a `BLOCKED` receipt, exactly as a precondition refusal does. **Every refusal the approver checks make is raised before `_take` for exactly this reason**: a check that refuses after the store call cannot say this, and the design that tried was rejected for it (§2.4.2). Not every refusal in §2 and §4 is one of @@ -295,7 +297,8 @@ only inside `_take`: | A human **denied** it | `ActionDenied(approval_denied)`, with `APPROVAL_DENIED`, `ACTION_DENIED` and a `DENIED` receipt | `ApprovalMismatch(approver_unverified)`, with `APPROVAL_INVALIDATED` and a `BLOCKED` receipt: a human's no stops appearing in the evidence as a no | | Already **consumed** (a replayed approval, G2) | `consumed` | `approver_unverified` | | The action **hash moved** (G1) | `mismatch`, which is `HASH_MISMATCH`'s value | `approver_unverified` | -| **Pending**, or no such approval | its own status | `approver_unverified` | +| **Pending** | `pending` | `approver_unverified` | +| **No such approval** | `unknown` | `approver_unverified` | So the gate stands aside for all four, the store's reason wins, and two shipped guarantees keep theirs. @@ -2022,8 +2025,10 @@ text editor, which is the point `v0.3 §5.5` makes about evaluation. **The check is at the consumption because `Control` never grants**, which §1.4 recorded and which building it confirmed: the only code that calls `grant_approval` outside a test is the CLI, the -operator server, `handle_inbound`, the scripted provider, the adapters, verify's own scenarios and -`Control._withdraw`. None of them is `Control` deciding anything. +operator server, `handle_inbound`, the scripted provider, the adapters and verify's own scenarios. +`Control._withdraw` calls `deny_approval`, which is the kernel closing a request it created rather +than a human answering one, and §2.6's table gives it its own row. None of them is `Control` +deciding anything. **The gate took three attempts and §2.4.2 records all three**, because the next reader will reach for one of the two that were wrong. Skipping the lapsed row is fail-open under clock skew: a diff --git a/src/ctrlrun/approval.py b/src/ctrlrun/approval.py index e63f1a3c..9c1e4d49 100644 --- a/src/ctrlrun/approval.py +++ b/src/ctrlrun/approval.py @@ -109,7 +109,26 @@ def __post_init__(self) -> None: if not self.agent: raise InvalidArgument("a verified approver must carry a non-empty agent") _require_aware(self.granted_at, "verified approver granted_at") - object.__setattr__(self, "entitled", tuple(self.entitled)) + # **`str` is a `Sequence`, and that is the whole of this check.** `tuple("abc")` is + # `("a", "b", "c")`, so a corrupted column holding a bare string became three control + # ids that entitle nothing and refuse nothing, and `_approvers_from_json`'s promise to + # raise on a corrupted row was quietly false. Same hazard §3.4 states for the roles + # claim, in a second place. + # Read as `object`, because the declared type is what a *caller* promises and this value + # arrives from a JSON column: mypy is right that a `tuple[str, ...]` cannot be a `str`, + # and a corrupted row is exactly the case where the declaration is not true. + given: object = self.entitled + if isinstance(given, str | bytes) or not isinstance(given, Iterable): + raise InvalidArgument( + f"a verified approver's 'entitled' must be a list of control ids, got " + f"{type(given).__name__}" + ) + entitled = tuple(given) + if not all(isinstance(item, str) and item for item in entitled): + raise InvalidArgument( + "a verified approver's 'entitled' must hold non-empty control ids" + ) + object.__setattr__(self, "entitled", entitled) @property def principal(self) -> tuple[str, str | None]: diff --git a/tests/test_approver.py b/tests/test_approver.py index 1a56f91b..3e0a5f77 100644 --- a/tests/test_approver.py +++ b/tests/test_approver.py @@ -695,6 +695,38 @@ def test_T296b_an_observed_denial_by_a_human_is_counted_too(store, clock): assert document["blocked_by_reason"] == {"approval_denied": 1} +def test_T283b_a_corrupted_entitled_column_is_refused_and_not_exploded(store, clock): + """`str` is a `Sequence`, so a corrupted row became three control ids rather than a refusal. + + `tuple("abc")` is `("a", "b", "c")`. A column holding a bare string was therefore accepted, + silently, as an approver entitled for three controls that do not exist, and + `_approvers_from_json`'s promise to raise on a corrupted row was false for exactly the shape + a corruption most easily takes. CodeRabbit found it; §3.4 states the same hazard for the + roles claim, which is where it was expected and not where it landed. + """ + from ctrlrun.approval import VerifiedApprover + from ctrlrun.errors import CTRLRunError + + for bad in ("abc", 5, [1], [""]): + with pytest.raises(CTRLRunError): + VerifiedApprover( + agent="human:bob", + user=None, + issuer=None, + granted_at=datetime.now(UTC), + entitled=bad, + ) + + fine = VerifiedApprover( + agent="human:bob", + user=None, + issuer=None, + granted_at=datetime.now(UTC), + entitled=["card-data-handling"], + ) + assert fine.entitled == ("card-data-handling",) + + # --- T297: the resumed leg is the only receipt some actions get ------------------------------ From 9214a607cbd826218e49f3c12c441b77c5386491 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 16:28:07 +0530 Subject: [PATCH 6/6] Say five where the table has five rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting the pending and unknown rows made §2.4.1's store-owned set five, and the prose around it still said four. CodeRabbit caught the count; fixing it showed that §10.2's T291b entry had drifted further than that, describing four rows when the tests assert six and calling the lapsed row "the fourth". §10.2 now describes what the tests do: the five store-owned rows, the lapsed row with a good approver, and a pointer to §2.4.2 for what checking the lapsed row costs. T291c gains its entry, including the two assertions that let it tell a refusal before the store call from one after, and T292 and T293 say what they cover and name T264 and T265 as the rest. Signed-off-by: arpan --- docs/SPEC-v0.8.md | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 24285e01..672df039 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -288,7 +288,7 @@ table and `BLOCKED_BY_STATE` both had to grow for. ### 2.4.1 Why the gate exists, and what it costs to omit it -Without it, this milestone would report the wrong reason for four refusals that have nothing to do +Without it, this milestone would report the wrong reason for five refusals that have nothing to do with approvers, because `Control` no longer applies `check_consumable` and the store applies it only inside `_take`: @@ -300,7 +300,7 @@ only inside `_take`: | **Pending** | `pending` | `approver_unverified` | | **No such approval** | `unknown` | `approver_unverified` | -So the gate stands aside for all four, the store's reason wins, and two shipped guarantees keep +So the gate stands aside for all five, the store's reason wins, and two shipped guarantees keep theirs. ### 2.4.2 The lapsed row, which took three attempts @@ -330,7 +330,7 @@ there is no clean cleanup: `fail_effect` requires `EXECUTING`, so `RESERVED → exist, and `mark_ambiguous` would assert "unknown" about an action known not to have run. **The answer is to check it, before `_take`, like the others.** The gate stands aside only for the -four rows above; the lapsed row is checked. +five rows above; the lapsed row is checked. | The row | What it reports | What it costs | |---|---|---| @@ -343,7 +343,7 @@ What is bought is that the lapsed row cannot be a way past §2.7 on a host whose and that **nothing is written by a refusal** stays literally true, which neither of the other two answers could say. -T291b covers the four rows and the lapsed-with-a-good-approver row; T291c is the skew reproduction +T291b covers the five store-owned rows and the lapsed-with-a-good-approver row; T291c is the skew reproduction and asserts the grant is still granted and nothing reserved; the lapsed-with-a-bad-approver row has its own test saying what it gives up. @@ -1508,18 +1508,25 @@ forbids (the third). provider anywhere** still reaches the approver checks: the test drives the 0.6-shaped path and asserts the refusal. Without this, every check in §2 to §4 is dead on the default path and every other test still passes (§2.4). -- **T291b:** the gate of §2.4.1, all four rows. With an `ApproverIdentity` configured and no +- **T291b:** the gate of §2.4.1, every row. With an `ApproverIdentity` configured and no `VerifiedApprover` on the row: a **denied** approval still raises `ActionDenied(approval_denied)` with `APPROVAL_DENIED` and a `DENIED` receipt; a **consumed** one still reports `consumed` (G2); - a **mutated action** still reports `mismatch` (G1); and a **granted but lapsed** one still - reports `expired`, appends exactly one `APPROVAL_EXPIRED`, and leaves the row moved to `expired` - by the store's own write. The fourth is the one a status-only gate fails: without it the lapse - has no event and no write, and the row stays `granted` for ever. -- **T292:** the migration, both directions, on SQLite and Postgres, from a database built by - 0.7.0's own code and not a hand-written fixture: rows with no approver columns open, migrate and - keep every value; an 0.7.0 binary against the migrated database refuses and names both versions. -- **T293:** a v3, v4 and v5 receipt chain verifies end to end, each receipt hashed by the rule its - own version wrote (`v0.7 §6.11`). + a **mutated action** still reports `mismatch` (G1); a **pending** one reports `pending` and an + **unknown** one `unknown`, which is the reason §2.7's fifth row and all of item 4's M-of-N + depend on; and a **granted but lapsed** one whose approver is fine still reports `expired`, + appends exactly one `APPROVAL_EXPIRED`, and leaves the row moved to `expired` by the store's own + write. The lapsed row is the one a status-only gate gets wrong in both directions, and §2.4.2 + is its argument: its own test asserts what checking it costs, which is that a grant both lapsed + **and** approver-refused keeps no `APPROVAL_EXPIRED` and no lapse write. +- **T291c:** the skew §2.4.2 exists for. A `Control` whose clock runs ahead of the store's presents + a self-approved grant: refused `approver_is_requester`, the executor not called, **the approval + still granted and nothing reserved**. Without the last two assertions the test cannot tell a + refusal before the store call from one after, which is what let the deferral look correct. +- **T292:** the migration ledger reaches `HEAD` and the column round-trips through a file-backed + store. The 0.6.1-built upgrade in both directions is `test_preconditions.py`'s T264, and §14.2 + says why this one does not duplicate it. +- **T293:** a chain carrying a `v5` receipt verifies end to end, and a `v4` document still parses + with neither `v5` key. The stored-v3-continued-by-this-binary chain is T265's. - **T294:** `ctrlrun.guarantees/v3` becomes `v4` here, and `verify` reports the catalogue version and G18, graded, never `N/A` (§11.7). - **T295:** the upgrade case of §2.9: an approval granted at 0.7.0, still pending, presented after