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..a47a2cc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,46 @@ 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 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 + 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. + + **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). + - **`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..672df039 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,14 +255,40 @@ 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. + +**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 +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 +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 -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`: @@ -271,53 +297,55 @@ 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** | `pending` | `approver_unverified` | +| **No such approval** | `unknown` | `approver_unverified` | + +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 + +**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 +five 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 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. ### 2.5 How a verified approver reaches the row, without a new store method @@ -439,6 +467,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` @@ -1462,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 @@ -1855,7 +1908,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 | @@ -1864,6 +1917,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 | @@ -1974,6 +2030,97 @@ 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 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 +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. + +**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 +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/__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..9c1e4d49 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, StaticIdentityProvider + +_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,166 @@ 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") + # **`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]: + """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 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 + # 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 +319,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..57d1dcc5 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -23,13 +23,18 @@ from .action import Action, Principal from .approval import ( + APPROVER_IS_REQUESTER, + APPROVER_UNVERIFIED, DEFAULT_APPROVAL_TTL, Approval, ApprovalProvider, ApprovalRecord, ApprovalRequest, ApprovalStatus, + ApprovalVerdict, + ApproverIdentity, LocalApprovalProvider, + VerifiedApprover, _precondition_at_request, _precondition_fingerprint, check_consumable, @@ -83,7 +88,6 @@ ) from .receipt import ( BLOCKED_AMBIGUOUS, - BLOCKED_APPROVAL_MISMATCH, BLOCKED_APPROVAL_REQUIRED, BLOCKED_ATTEMPT_CEILING, BLOCKED_DUPLICATE, @@ -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). @@ -1155,7 +1176,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) self._append( EventType.APPROVAL_INVALIDATED, action, @@ -1222,12 +1251,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 @@ -1375,6 +1410,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( @@ -2526,15 +2567,101 @@ def _recheck( compared.reset() record = self._store.get_approval(approval_id) stored = None if record is None else record.request.precondition_fingerprint - if preconditions is None and stored is None: - return 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, verdict) + if preconditions is None and stored is None: + return 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, + verdict: ApprovalVerdict, + ) -> 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 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 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 + # where no approver identity is configured and nothing was refused. + compared.approvers = record.approvers + if self._approver_identity is None: + return + 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 + # **`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, ...] + ) -> 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 " + "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, @@ -2896,6 +3023,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=() 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..7706f798 100644 --- a/src/ctrlrun/postgres.py +++ b/src/ctrlrun/postgres.py @@ -46,6 +46,7 @@ ApprovalRecord, ApprovalRequest, ApprovalStatus, + _verified_approver_now, check_answerable, check_consumable, ) @@ -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..28edab65 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -22,6 +22,12 @@ 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 APPROVAL_DENIED as APPROVAL_DENIED_REASON +from .approval import VerifiedApprover from .errors import CTRLRunError, InvalidArgument from .policy import Decision @@ -35,12 +41,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 +76,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 @@ -99,7 +110,43 @@ #: 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 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 +#: 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. +#: **`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", + "unknown", + APPROVAL_DENIED_REASON, + "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( { @@ -108,6 +155,7 @@ BLOCKED_IN_PROGRESS, BLOCKED_AMBIGUOUS, BLOCKED_ATTEMPT_CEILING, + *BLOCKED_APPROVAL_REASONS, } ) @@ -372,6 +420,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 +513,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 +582,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 +604,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..fe2c5fb0 100644 --- a/src/ctrlrun/state.py +++ b/src/ctrlrun/state.py @@ -37,6 +37,8 @@ ApprovalRequest, ApprovalStatus, ApprovalStore, + VerifiedApprover, + _verified_approver_now, check_answerable, check_consumable, ) @@ -733,6 +735,42 @@ 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). + + **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 () + 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: """Everything held in process memory: for tests and `ctrlrun demo`. @@ -872,11 +910,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 +929,13 @@ def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) with self._lock: record = self._answerable(approval_id) + now = self._clock() + verified = _verified_approver_now(now) 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 +1591,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 +1625,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 +1681,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..5031bf66 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. `_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) @@ -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..3e0a5f77 --- /dev/null +++ b/tests/test_approver.py @@ -0,0 +1,849 @@ +"""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_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) + + 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" + ) + + # 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 ------------------------------------------------------------ + + +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_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) + _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" + + +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" + + +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 ------------------------------------------ + + +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 + # **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 -------------------------------------------------------------------- + + +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 _observing(store, clock, identity): + return Control( + Policy.from_yaml(OBSERVE_POLICY), + store, + clock=clock, + approver_identity=identity, + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + + +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() + + with with_approval(request_id): + receipt = _observing(store, clock, identity).execute(action, executor, KEY) + + 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" + ) + + +# --- 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} + + +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} + + +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 ------------------------------ + + +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_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) + _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 == "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_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) + request_id = _requested(control, action) + _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") + 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..11642eb5 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) @@ -987,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) @@ -1411,12 +1416,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 +1562,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) @@ -2070,10 +2081,10 @@ 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/v3" + assert reg.CATALOGUE == "ctrlrun.guarantees/v4" assert "G16" in reg.BY_ID assert reg.BY_ID["G16"].descends_from @@ -2323,6 +2334,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 +2342,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}