From c4fc4b0760a4115037c044e0daae3af36d867c58 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 11:41:56 +0530 Subject: [PATCH 1/2] Revoke by selector: everything a principal issued, everything under a grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ctrlrun revoke takes one id, and the ids are in the events file. 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 under pressure. --created-by PRINCIPAL and --under ID are queries over rows that already exist. No new StateStore method: delegations(include_revoked=True) and revoke_delegation are what they read and call, and the filter sits above the store. Each match is revoked exactly as one id is, one at a time, through the same Control.revoke, so a run that stops halfway leaves the rows it reached revoked and the rest untouched, and a second run finishes. --by is unchanged. The roadmap called the new selector --by , which is the opposite meaning on an option that already exists and records who performed the revocation, so the selector is --created-by and every script written against 0.7.0 keeps working. An empty selector exits non-zero and names what it searched for: during an incident a mistyped name that exits 0 reads as a finished job. --under is strictly beneath, so it leaves the id it names alone; an operator who wants that row as well already has ctrlrun revoke . Still no unrevoke. T272 to T280, on both backends, and the killed run is a real command in its own process killed on its first recorded revocation. Two things building it settled, in SPEC-v0.8 §14.1: a kill between the row write and the event append leaves a revoked row with no event, which is v0.3 behaviour rather than anything the selector adds and which T276 bounds rather than assumes; and two guards were green under mutation because a later guard fired with the same exit code, so the tests now assert which one ran. Signed-off-by: arpan --- CHANGELOG.md | 24 ++ docs/SPEC-v0.8.md | 41 +++ src/ctrlrun/cli/main.py | 142 ++++++++++- tests/test_revoke_selector.py | 453 ++++++++++++++++++++++++++++++++++ 4 files changed, 655 insertions(+), 5 deletions(-) create mode 100644 tests/test_revoke_selector.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b9b39114..85b7a213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,30 @@ any change to one appears here. ## [Unreleased] +### Added + +- **`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 + under pressure. Both are queries over rows that already exist: no new `StateStore` method, no + bulk statement, and no transaction over the set. **Each match is revoked exactly as one id is**, + one at a time, so a run that stops halfway leaves the rows it reached revoked and the rest + untouched, and a second run finishes. `--created-by` takes `AGENT` or `AGENT/USER`, splitting on + the first `/` as `ctrlrun delegate --as` does; `--under` reaches the subtree at every depth and + is strictly beneath, so it leaves the id it names alone. A selector that matches nothing **exits + non-zero** and says what it searched for, because during an incident a mistyped name that exits 0 + reads as a finished job. Still no `unrevoke`, in any costume. + + **`--by` is unchanged and still means who performed the revocation.** The roadmap called the new + selector `--by `, which is the opposite meaning on an option that already exists, so + the selector is `--created-by` and every script written against 0.7.0 keeps working. + + **What a killed run can leave, stated because the tests bound it rather than assume it:** a + revoked row whose `DELEGATION_REVOKED` event was never written. `Control.revoke` writes the row + and then appends the event, with no transaction over the pair, so a `SIGKILL` between them leaves + one row unaccounted for in the log. That is 0.3 behaviour for a single `ctrlrun revoke` too; a + selector only makes the window easy to land in. + ### Documentation - **`docs/SPEC-v0.8.md`**: the v0.8 "Oversight" contract, a delta over v0.1 to v0.7. No code lands diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 42c7b350..40471951 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -1931,6 +1931,47 @@ arguments are written down as they are decided, not afterwards. ### 14.1 Item 1: revocation by selector +**A killed run can leave a revoked row with no event, and that is v0.3 behaviour rather than +anything the selector adds.** `Control.revoke` writes the row and then appends the event: two +writes, no transaction over the pair, and `StateStore` is frozen (`v0.6 §9.2`). A `SIGKILL` +between them leaves the delegation revoked, correctly and durably, with no `DELEGATION_REVOKED` +behind it. A selector run does not create the window; it makes it easy to land in, because it +takes the same two writes two hundred times instead of once. T276 asserts the bound it can +assert, that **at most one** row is missing its event, and the first draft of that test asserted +equality, which is stricter than the code has ever promised. Closing it needs the row and the +event in one store call, which is a store method, which is the maintainer's call and not an +item's. + +**Two guards were green under mutation, and both were subsumed rather than absent.** The +mutation table caught them, which is what it is for. + +- Removing the refusal of `--created-by a/b/c` left T278 green, because `a/b/c` then parses as + the agent `a` and the user `b/c`, matches nothing, and exits non-zero through §7.5's + empty-selector error instead. The test asserted an exit code where it had to assert a message. +- Removing the skip over already-revoked rows left T275 green, because `Control.revoke` is + idempotent and appends no second event whatever the loop does. What the skip is actually for + is the terminal: without it a rerun prints `revoked ` for two hundred rows it did not + revoke. The test now asserts that line's absence, which is the only thing that makes the skip + load-bearing. + +Both are `CONTRIBUTING.md`'s first shape of a false green, and both would have read as covered. + +**`--under` is strictly beneath.** A row is not under itself, so `--under ` +revokes that delegation's descendants and leaves the delegation alone. The alternative reading +costs nothing to implement and is worse to use: an operator who wants the row as well already has +`ctrlrun revoke `, and one who wanted only the subtree would have no way back. + +**Polling the store to trigger the kill made T276 pass for the wrong reason.** Opening a +`StateStore` per poll costs more than the two hundred revocations it is watching, so the kill +landed after the child had finished and the test asserted its window had been opened when it had +not. It polls the JSONL event file instead, which is a read of a file the command is already +appending to. + +**No new `StateStore` method, and none was tempting.** `delegations(include_revoked=True)` and +`revoke_delegation` already exist, the filter is twenty lines above the store, and the subtree +walk is bounded by the rows it has already seen because a cycle is reachable with `sqlite3` and a +text editor, which is the point `v0.3 §5.5` makes about evaluation. + ### 14.2 Item 2: the approver is a principal ### 14.3 Item 3: entitlement from the control registry diff --git a/src/ctrlrun/cli/main.py b/src/ctrlrun/cli/main.py index 2c78747c..ecacd76c 100644 --- a/src/ctrlrun/cli/main.py +++ b/src/ctrlrun/cli/main.py @@ -36,7 +36,7 @@ verify_chain, ) from ..reporting import inspection_for, since_boundary, stats_document -from ..state import RESOLUTIONS, SQLiteStateStore, StateStore +from ..state import RESOLUTIONS, DelegationRecord, SQLiteStateStore, StateStore from .demo import run_demo #: Who the CLI records as the answer's author. Free text in v0.1 (SPEC-v0.1 §4.1). @@ -931,22 +931,80 @@ def delegate( @main.command() -@click.argument("delegation_id") +@click.argument("delegation_id", required=False) @click.option("--by", "by", default=CLI_APPROVER, show_default=True, help="Who revoked it.") +@click.option( + "--created-by", + "created_by", + default=None, + help="Revoke every delegation this principal created: AGENT or AGENT/USER.", +) +@click.option( + "--under", + "under", + default=None, + help="Revoke every delegation beneath this grant or delegation id, at any depth.", +) @STORE_URL_OPTION -def revoke(delegation_id: str, by: str, store_url: str | None) -> None: +def revoke( + delegation_id: str | None, + by: str, + created_by: str | None, + under: str | None, + store_url: str | None, +) -> None: """Revoke a delegation, and with it every delegation beneath it. Transitive by structure and not reversible: there is no `unrevoke`, because the operation whose safety matters is the one taken in a hurry (SPEC-v0.3 §5.7). Revoking an already-revoked delegation is idempotent and exits 0. + + `--created-by` and `--under` are selectors over rows that already exist (SPEC-v0.8 §7). + Each match is revoked **exactly as one id is**: one revocation, one record, one event, in + turn, so a run that stops halfway leaves the rows it reached revoked and the rest untouched, + and a second run finishes. A selector that matches nothing exits non-zero (§7.5), because + during an incident a mistyped name that exits 0 reads as "done". + + `--by` is unchanged and means what it has always meant: who performed the revocation. """ + selected = _one_selector(delegation_id, created_by, under) try: control = _control_on(store_url) - before = control.store.get_delegation(delegation_id) - control.revoke(delegation_id, by=by) + if selected is None: + _revoke_one(control, str(delegation_id), by) + return + _revoke_selected(control, selected, by) except CTRLRunError as exc: raise _fail(exc) from exc + + +def _one_selector( + delegation_id: str | None, created_by: str | None, under: str | None +) -> tuple[str, str] | None: + """`None` for a single id, or the one selector given, as `(kind, value)` (SPEC-v0.8 §7.2). + + Two ways of naming what to revoke in one invocation is a command whose blast radius depends + on which the reader believes, so every combination is a usage error rather than a precedence + rule nobody would remember at 3am. + """ + named = [name for name, value in (("--created-by", created_by), ("--under", under)) if value] + if len(named) > 1: + raise click.UsageError("--created-by and --under name different sets; give one of them") + if named and delegation_id is not None: + raise click.UsageError( + f"{named[0]} selects the rows to revoke, so a delegation id cannot be given as well" + ) + if not named: + if delegation_id is None: + raise click.UsageError("give a delegation id, or --created-by PRINCIPAL, or --under ID") + return None + return ("--created-by", created_by) if created_by else ("--under", str(under)) + + +def _revoke_one(control: Control, delegation_id: str, by: str) -> None: + """One id, exactly as v0.3 §5.7 revoked it, and the shape a selector run repeats.""" + before = control.store.get_delegation(delegation_id) + control.revoke(delegation_id, by=by) if before is not None and before.revoked_at is not None: click.echo(f"{delegation_id} was already revoked at {iso_timestamp(before.revoked_at)}") return @@ -954,6 +1012,80 @@ def revoke(delegation_id: str, by: str, store_url: str | None) -> None: click.echo("every delegation beneath it is denied from the next evaluation") +def _revoke_selected(control: Control, selected: tuple[str, str], by: str) -> None: + """Every row the selector matches, one at a time (SPEC-v0.8 §7.3, §7.4).""" + kind, value = selected + rows = control.store.delegations(include_revoked=True) + matched = _created_by(rows, value) if kind == "--created-by" else _beneath(rows, value) + if not matched: + raise click.ClickException( + f"no delegation matched {kind} {value!r}; nothing was revoked. A selector that " + "matched nothing exits non-zero so a mistyped name does not read as a finished job" + ) + already = [record for record in matched if record.is_revoked] + for record in matched: + if record.is_revoked: + continue + # One at a time, through the same call a single id takes: a bulk write would leave a + # killed run with rows nobody can account for, and there is no transaction over the set. + control.revoke(record.delegation_id, by=by) + click.echo(f"revoked {record.delegation_id} by {by}") + click.echo( + f"revoked {len(matched) - len(already)} of {len(matched)} matching " + f"{kind} {value}; {len(already)} already revoked" + ) + click.echo("every delegation beneath them is denied from the next evaluation") + + +def _created_by(rows: tuple[DelegationRecord, ...], value: str) -> list[DelegationRecord]: + """The rows this principal created: AGENT, or AGENT/USER (SPEC-v0.8 §7.3). + + Split on the first '/', as `delegate --as` splits, and refusing the same thing it refuses: + a name with two separators is ambiguous, and a selector nobody can read is one that revokes + the wrong subtree during an incident. + """ + agent, separator, user = value.partition("/") + if not agent: + raise click.UsageError("--created-by needs an agent name: AGENT or AGENT/USER") + if "/" in user: + raise click.UsageError( + f"--created-by {value!r} has more than one '/': write AGENT or AGENT/USER, and note " + "that an agent name containing '/' cannot be written, as 'delegate --as' says" + ) + return [ + record + for record in rows + if record.created_by_agent == agent and (not separator or record.created_by_user == user) + ] + + +def _beneath(rows: tuple[DelegationRecord, ...], parent_id: str) -> list[DelegationRecord]: + """Every row whose parent chain reaches `parent_id`, at any depth (SPEC-v0.8 §7.3). + + Strictly beneath: a row is not under itself, so `--under ` revokes that + delegation's descendants and leaves it alone, which is what the words say. + + The walk is bounded by the rows it has already seen, because a chain edited into a cycle + with `sqlite3` and a text editor is reachable (SPEC-v0.3 §5.5 makes the same point about + evaluation) and an incident command must not hang on one. + """ + by_id = {record.delegation_id: record for record in rows} + matched = [] + for record in rows: + seen: set[str] = {record.delegation_id} + current = record.parent_id + while current not in seen: + if current == parent_id: + matched.append(record) + break + seen.add(current) + parent = by_id.get(current) + if parent is None: + break + current = parent.parent_id + return matched + + def _delegation_dict(delegation: Delegation) -> dict[str, Any]: """One delegation as portable JSON, for `ctrlrun delegate --json` (SPEC-v0.3 §5.2).""" return { diff --git a/tests/test_revoke_selector.py b/tests/test_revoke_selector.py new file mode 100644 index 00000000..aab73955 --- /dev/null +++ b/tests/test_revoke_selector.py @@ -0,0 +1,453 @@ +"""T272 to T280: `ctrlrun revoke --created-by` and `--under` (SPEC-v0.8 §7). + +The selector is a query over rows that already exist. Every match is revoked exactly as one id +is revoked today, one at a time, so a run that stops halfway leaves the rows it reached revoked +and the rest untouched (§7.3, §7.4). +""" + +from __future__ import annotations + +import json +import os +import signal +import subprocess +import sys +import time +import uuid +from datetime import UTC, datetime + +import pytest +from click.testing import CliRunner + +from ctrlrun.cli.main import main +from ctrlrun.state import SQLiteStateStore + +#: Every test here builds an `authority:` section and writes `DELEGATION_*` events, which +#: `conftest` requires a test to declare (SPEC-v0.3 §4.1). +pytestmark = pytest.mark.authority + +# --- the workspace ---------------------------------------------------------------------------- + +DOCUMENT = """schema: ctrlrun.policy/v3 +environment: production +authority: + max_delegation_depth: 4 + grants: + - id: head-of-finance + subject: { agent: "human-cfo" } + actions: ["stripe.**"] + resources: ["payment:EU-*"] + constraints: { amount_gte: 0, amount_lte: 10000000 } + environments: ["production"] + expires_at: "2027-01-01T00:00:00+00:00" + delegable: true + - id: ops-lead + subject: { agent: "human-ops" } + actions: ["deploy.**"] + resources: ["service:EU-*"] + constraints: { replicas_gte: 0, replicas_lte: 100 } + environments: ["production"] + expires_at: "2027-01-01T00:00:00+00:00" + delegable: true +actions: + stripe.refund: + decision: allow + deploy.rollout: + decision: allow +""" + +FINANCE_CHILD = """subject: { agent: "finance-agent", user: "cfo@example.com" } +actions: ["stripe.refund"] +resources: ["payment:EU-4*"] +constraints: { amount_gte: 0, amount_lte: 2500000 } +environments: ["production"] +expires_at: "2026-12-01T00:00:00+00:00" +delegable: true +""" + +FINANCE_GRANDCHILD = """subject: { agent: "finance-agent", user: "cfo@example.com" } +actions: ["stripe.refund"] +resources: ["payment:EU-42"] +constraints: { amount_gte: 0, amount_lte: 1000 } +environments: ["production"] +expires_at: "2026-12-01T00:00:00+00:00" +delegable: true +""" + +OPS_CHILD = """subject: { agent: "deploy-agent", user: "ops@example.com" } +actions: ["deploy.rollout"] +resources: ["service:EU-4*"] +constraints: { replicas_gte: 0, replicas_lte: 10 } +environments: ["production"] +expires_at: "2026-12-01T00:00:00+00:00" +delegable: true +""" + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + + +@pytest.fixture( + params=[ + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + not POSTGRES_URL, + reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against", + ), + ), + ] +) +def workspace(request, tmp_path, monkeypatch): + """A working tree with a policy and the grant files, on either backend (T280). + + The Postgres run gets a scratch schema of its own, migrated by opening a store once and + dropped afterwards, because `ctrlrun` refuses a schema that is not already at HEAD. + """ + (tmp_path / "ctrlrun.yaml").write_text(DOCUMENT, encoding="utf-8") + (tmp_path / "finance-child.yaml").write_text(FINANCE_CHILD, encoding="utf-8") + (tmp_path / "finance-grandchild.yaml").write_text(FINANCE_GRANDCHILD, encoding="utf-8") + (tmp_path / "ops-child.yaml").write_text(OPS_CHILD, encoding="utf-8") + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("CTRLRUN_CONFIG", raising=False) + monkeypatch.delenv("CTRLRUN_STATE", raising=False) + + if request.param == "sqlite": + yield _Workspace(tmp_path, []) + return + + from ctrlrun.postgres import PostgresStateStore + + schema = f"revsel_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + migrated = PostgresStateStore(POSTGRES_URL, schema=schema) + migrated.close() + url = f"{POSTGRES_URL}?ctrlrun_schema={schema}" + try: + yield _Workspace(tmp_path, ["--store-url", url]) + finally: + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + +class _Workspace: + def __init__(self, path, store_args): + self.path = path + self.store_args = store_args + + def cli(self, *arguments): + return CliRunner().invoke(main, [*arguments, *self.store_args]) + + def delegate(self, parent: str, as_who: str, grant: str = "finance-child.yaml") -> str: + result = self.cli("delegate", "--parent", parent, "--file", grant, "--as", as_who, "--json") + assert result.exit_code == 0, result.output + return str(json.loads(result.output)["delegation_id"]) + + def records(self): + """Every delegation row, revoked or not, keyed by id.""" + store = self._open() + try: + return {row.delegation_id: row for row in store.delegations(include_revoked=True)} + finally: + store.close() + + def revoked_events(self): + store = self._open() + try: + return [event for event in store.events() if event.type == "DELEGATION_REVOKED"] + finally: + store.close() + + def _open(self): + if not self.store_args: + return SQLiteStateStore(self.path / ".ctrlrun" / "state.db") + from ctrlrun.postgres import PostgresStateStore + + url = self.store_args[1] + bare, _, schema = url.partition("?ctrlrun_schema=") + return PostgresStateStore(bare, schema=schema) + + +@pytest.fixture +def tree(workspace): + """Six delegations: five beneath `head-of-finance` at three depths, one beneath `ops-lead`. + + `alice`, `bob` and a user-less `human-cfo` create the three top rows, so T273 can tell + `--created-by AGENT` from `--created-by AGENT/USER` on rows that differ only in the user. + """ + alice = workspace.delegate("head-of-finance", "human-cfo/alice@example.com") + bob = workspace.delegate("head-of-finance", "human-cfo/bob@example.com") + bare = workspace.delegate("head-of-finance", "human-cfo") + ops = workspace.delegate("ops-lead", "human-ops/carol@example.com", "ops-child.yaml") + child = workspace.delegate(alice, "finance-agent/cfo@example.com", "finance-grandchild.yaml") + grandchild = workspace.delegate( + child, "finance-agent/cfo@example.com", "finance-grandchild.yaml" + ) + return { + "alice": alice, + "bob": bob, + "bare": bare, + "ops": ops, + "child": child, + "grandchild": grandchild, + } + + +def _revoked(workspace, ids): + rows = workspace.records() + return {name for name, identifier in ids.items() if rows[identifier].is_revoked} + + +# --- T272: --created-by matches the creator, and nothing else --------------------------------- + + +def test_T272_created_by_revokes_that_creators_rows_and_leaves_every_other(workspace, tree): + """§7.3 — three creators in one store, and the selector reaches exactly one of them.""" + result = workspace.cli("revoke", "--created-by", "human-ops") + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"ops"} + + +def test_T272_the_other_direction_is_asserted_too(workspace, tree): + """A selector that revoked everything would pass the assertion above on its own.""" + result = workspace.cli("revoke", "--created-by", "finance-agent/cfo@example.com") + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"child", "grandchild"} + + +# --- T273: AGENT and AGENT/USER are different selectors --------------------------------------- + + +def test_T273_created_by_agent_matches_every_user(workspace, tree): + """§7.3 — `--created-by AGENT` matches rows created by that agent with any user, or none.""" + result = workspace.cli("revoke", "--created-by", "human-cfo") + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"alice", "bob", "bare"} + + +def test_T273_created_by_agent_slash_user_matches_both_fields(workspace, tree): + """The same rows, told apart by the user: only `alice`'s row is reached.""" + result = workspace.cli("revoke", "--created-by", "human-cfo/alice@example.com") + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"alice"} + + +# --- T274: --under reaches the subtree at every depth ----------------------------------------- + + +def test_T274_under_revokes_the_subtree_including_a_grandchild(workspace, tree): + """§7.3 — the subtree of a policy grant with several children, at every depth.""" + result = workspace.cli("revoke", "--under", "head-of-finance") + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"alice", "bob", "bare", "child", "grandchild"} + + +def test_T274_under_a_delegation_reaches_its_own_subtree_only(workspace, tree): + """`--under` takes a delegation id as readily as a grant id, and stops where the tree does.""" + result = workspace.cli("revoke", "--under", tree["child"]) + + assert result.exit_code == 0, result.output + assert _revoked(workspace, tree) == {"grandchild"} + + +# --- T275: one event per match, and a second run is idempotent -------------------------------- + + +def test_T275_each_match_produces_one_event_and_a_second_run_produces_none(workspace, tree): + """§7.4 — one revocation, one record, per row, exactly as one id is revoked today.""" + first = workspace.cli("revoke", "--created-by", "human-cfo") + assert first.exit_code == 0, first.output + + after_first = workspace.revoked_events() + assert len(after_first) == 3 + assert {event.data["delegation_id"] for event in after_first} == { + tree["alice"], + tree["bob"], + tree["bare"], + } + + second = workspace.cli("revoke", "--created-by", "human-cfo") + + assert second.exit_code == 0, second.output + assert len(workspace.revoked_events()) == 3 + assert "3 already revoked" in second.output + # **And it says nothing it did not do.** `Control.revoke` is idempotent, so a second pass + # over revoked rows appends no event whatever the command does, and the event count above is + # green with the skip deleted. What the skip is actually for is the terminal: without it a + # rerun prints "revoked " for every row it did not revoke. Asserting the absence is what + # makes the guard load-bearing (`CONTRIBUTING.md`'s first shape of a false green). + assert "revoked " + tree["alice"] not in second.output + + +# --- T276: a run that stops halfway ----------------------------------------------------------- + + +CHILD = """ +import sys +from ctrlrun.cli.main import main + +sys.exit(main(["revoke", "--created-by", "human-cfo"] + sys.argv[1:], standalone_mode=True)) +""" + + +@pytest.mark.skipif(sys.platform == "win32", reason="SIGKILL is not available") +def test_T276_a_killed_run_leaves_what_it_reached_revoked_and_a_second_run_finishes( + workspace, tmp_path +): + """§7.4 — a real interruption of a real command, bounded, and the window opened on purpose. + + The rows are created first, the command is spawned as its own process, and it is killed as + soon as the store shows its first revocation. A test that called an internal function would + prove nothing about what a `Ctrl-C` at 3am leaves behind. + """ + for index in range(200): + workspace.delegate("head-of-finance", f"human-cfo/user-{index}@example.com") + + script = tmp_path / "run_revoke.py" + script.write_text(CHILD, encoding="utf-8") + events = workspace.path / ".ctrlrun" / "events.jsonl" + before = events.read_text(encoding="utf-8").count('"DELEGATION_REVOKED"') + + child = subprocess.Popen( + [sys.executable, str(script), *workspace.store_args], + cwd=str(workspace.path), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + try: + # The event file, not the store: opening a store to poll costs more than the run it is + # watching, and the kill then lands after the child has finished, which opens no window. + deadline = time.monotonic() + 30 + while time.monotonic() < deadline: + if events.read_text(encoding="utf-8").count('"DELEGATION_REVOKED"') > before: + break + if child.poll() is not None: + break + time.sleep(0.001) + child.send_signal(signal.SIGKILL) + finally: + child.wait(timeout=30) + + rows = workspace.records() + revoked = [row for row in rows.values() if row.is_revoked] + assert revoked, "the child revoked nothing, so the run never started" + assert len(revoked) < len(rows), ( + "the child finished before it could be killed, so no window was opened: " + f"{len(revoked)} of {len(rows)}" + ) + # **At most one row can lack its event, and the reason is worth stating.** `Control.revoke` + # writes the row and then appends the event: two writes, no transaction over the pair, and + # `StateStore` is frozen (SPEC-v0.6 §9.2), so a kill between them leaves a revoked row whose + # event was never written. That is v0.3 behaviour for a single `ctrlrun revoke` as much as + # for a selector; a selector only makes the window easier to land in. Asserting equality + # here would be asserting something the code does not promise. + events = len(workspace.revoked_events()) + assert len(revoked) - 1 <= events <= len(revoked), ( + f"{events} events for {len(revoked)} revoked rows: at most one row may be mid-write" + ) + + second = workspace.cli("revoke", "--created-by", "human-cfo") + + assert second.exit_code == 0, second.output + after = workspace.records() + assert all(row.is_revoked for row in after.values() if row.created_by_agent == "human-cfo") + + +# --- T277: an empty selector is an error ------------------------------------------------------ + + +def test_T277_a_selector_matching_nothing_exits_non_zero_and_names_what_it_searched_for( + workspace, tree +): + """§7.5 — the one place in v0.8 where an empty result is a failure.""" + missing = workspace.cli("revoke", "--created-by", "human-cfo-typo") + + assert missing.exit_code != 0 + assert "human-cfo-typo" in missing.output + assert _revoked(workspace, tree) == set() + + nowhere = workspace.cli("revoke", "--under", "no-such-grant") + + assert nowhere.exit_code != 0 + assert "no-such-grant" in nowhere.output + + +# --- T278: the usage errors ------------------------------------------------------------------- + + +def test_T278_the_selectors_are_mutually_exclusive_and_refuse_a_positional_id(workspace, tree): + """§7.2 — each combination is a usage error with its own message.""" + both = workspace.cli("revoke", "--created-by", "human-cfo", "--under", "head-of-finance") + assert both.exit_code != 0 + assert "--created-by" in both.output and "--under" in both.output + + with_id = workspace.cli("revoke", tree["alice"], "--created-by", "human-cfo") + assert with_id.exit_code != 0 + assert "--created-by" in with_id.output + + under_with_id = workspace.cli("revoke", tree["alice"], "--under", "head-of-finance") + assert under_with_id.exit_code != 0 + assert "--under" in under_with_id.output + + neither = workspace.cli("revoke") + assert neither.exit_code != 0 + + assert _revoked(workspace, tree) == set() + + +def test_T278_an_agent_name_containing_a_slash_cannot_be_written(workspace, tree): + """The rule `delegate --as` already states, on the selector that reads what it wrote. + + **The message is asserted, not only the exit code**, and the mutation table is why: with + the two-separator refusal removed, `a/b/c` parses as the agent `a` and the user `b/c`, + matches nothing, and exits non-zero through §7.5's empty-selector error instead. A test + that asserted only the exit code was green with the guard deleted, which is a subsumed + guard (`CONTRIBUTING.md`'s first shape of a false green). + """ + result = workspace.cli("revoke", "--created-by", "a/b/c") + + assert result.exit_code != 0 + assert "more than one '/'" in result.output + assert "no delegation matched" not in result.output + assert _revoked(workspace, tree) == set() + + +# --- T279: --by keeps its 0.7.0 meaning ------------------------------------------------------- + + +def test_T279_by_still_names_who_performed_the_revocation(workspace, tree): + """§7.2 — two meanings on one option is the defect; `--by` keeps the one it had.""" + selector = workspace.cli("revoke", "--created-by", "human-ops", "--by", "incident-4412") + assert selector.exit_code == 0, selector.output + + single = workspace.cli("revoke", tree["alice"], "--by", "incident-4412") + assert single.exit_code == 0, single.output + + rows = workspace.records() + assert rows[tree["ops"]].revoked_by == "incident-4412" + assert rows[tree["alice"]].revoked_by == "incident-4412" + assert all(event.data["revoked_by"] == "incident-4412" for event in workspace.revoked_events()) + + +def test_T279_by_defaults_to_the_cli_approver_on_a_selector_run(workspace, tree): + """The default is the one `ctrlrun revoke ` has always had.""" + result = workspace.cli("revoke", "--created-by", "human-ops") + + assert result.exit_code == 0, result.output + assert workspace.records()[tree["ops"]].revoked_by == "cli:local" + + +# --- T280 is the `workspace` fixture: every test above runs on both backends ------------------- + + +def test_T280_the_backend_under_test_is_the_one_the_command_wrote_to(workspace, tree): + """The positive control for the fixture itself: a vacuous backend parameter proves nothing.""" + rows = workspace.records() + + assert set(tree.values()) <= set(rows) + assert all(row.created_at.tzinfo is not None for row in rows.values()) + assert datetime.now(UTC) >= max(row.created_at for row in rows.values()) From 4880df54d3051bfae2002374dfb62b9117984d16 Mon Sep 17 00:00:00 2001 From: arpan Date: Sat, 12 Sep 2026 11:58:02 +0530 Subject: [PATCH 2/2] Answer CodeRabbit: a trailing separator, and a test that could write outside its schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both real. --created-by "human-cfo/" set the separator and left the user empty, which the filter read as "the user is the empty string", matched nothing, and reported §7.5's no-match error. A typed-and-lost user is not "every user", and the widest reading of an ambiguous command is the wrong default during an incident, so it is a usage error that names the two forms. The message is asserted, not the exit code, because without the guard the command exits non-zero anyway through the no-match error: the same subsumed shape the mutation table caught twice in this item already. M9a covers it. And the worse one, in the test rather than the code. The Postgres fixture built its store URL with an f-string, so a CTRLRUN_TEST_POSTGRES carrying "?sslmode=require" produced a second "?", _peel_schema read ctrlrun_schema as part of the sslmode value, and the schema fell back to public: the test would have passed while the command wrote outside its scratch schema, into the database every other test shares. The URL is now built with urllib.parse, and _Workspace keeps the base URL and the schema apart rather than re-parsing them out of the CLI's own argument, since a test that reads a different schema from the one the command wrote to asserts nothing. Ten mutations now, all caught. Signed-off-by: arpan --- src/ctrlrun/cli/main.py | 8 +++++++ tests/test_revoke_selector.py | 42 ++++++++++++++++++++++++++++++----- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/ctrlrun/cli/main.py b/src/ctrlrun/cli/main.py index ecacd76c..e77a911d 100644 --- a/src/ctrlrun/cli/main.py +++ b/src/ctrlrun/cli/main.py @@ -1047,6 +1047,14 @@ def _created_by(rows: tuple[DelegationRecord, ...], value: str) -> list[Delegati agent, separator, user = value.partition("/") if not agent: raise click.UsageError("--created-by needs an agent name: AGENT or AGENT/USER") + if separator and not user: + # `--created-by agent/` is a typed-and-lost user, not "any user": reading it as the + # latter would revoke every row that agent created, which is the widest reading of an + # ambiguous command during an incident. Refused rather than guessed. + raise click.UsageError( + f"--created-by {value!r} ends with '/': write AGENT for every user, or AGENT/USER " + "for one" + ) if "/" in user: raise click.UsageError( f"--created-by {value!r} has more than one '/': write AGENT or AGENT/USER, and note " diff --git a/tests/test_revoke_selector.py b/tests/test_revoke_selector.py index aab73955..3fe947fa 100644 --- a/tests/test_revoke_selector.py +++ b/tests/test_revoke_selector.py @@ -15,6 +15,7 @@ import time import uuid from datetime import UTC, datetime +from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit import pytest from click.testing import CliRunner @@ -122,17 +123,30 @@ def workspace(request, tmp_path, monkeypatch): PostgresStateStore.create_schema(POSTGRES_URL, schema) migrated = PostgresStateStore(POSTGRES_URL, schema=schema) migrated.close() - url = f"{POSTGRES_URL}?ctrlrun_schema={schema}" try: - yield _Workspace(tmp_path, ["--store-url", url]) + yield _Workspace(tmp_path, ["--store-url", _with_schema(POSTGRES_URL, schema)], schema) finally: PostgresStateStore.drop_schema(POSTGRES_URL, schema) +def _with_schema(url: str, schema: str) -> str: + """`url` with CTRLRun's schema parameter added, keeping every parameter it already has. + + A naive f-string appends a second '?' to a URL carrying `?sslmode=require`, and + `_peel_schema` then reads `ctrlrun_schema` as part of the `sslmode` value and falls back to + `public`. The test would pass while the command wrote outside its scratch schema, into the + database every other test shares. + """ + parts = urlsplit(url) + pairs = [*parse_qsl(parts.query), ("ctrlrun_schema", schema)] + return urlunsplit(parts._replace(query=urlencode(pairs))) + + class _Workspace: - def __init__(self, path, store_args): + def __init__(self, path, store_args, schema=None): self.path = path self.store_args = store_args + self.schema = schema def cli(self, *arguments): return CliRunner().invoke(main, [*arguments, *self.store_args]) @@ -162,9 +176,10 @@ def _open(self): return SQLiteStateStore(self.path / ".ctrlrun" / "state.db") from ctrlrun.postgres import PostgresStateStore - url = self.store_args[1] - bare, _, schema = url.partition("?ctrlrun_schema=") - return PostgresStateStore(bare, schema=schema) + # The base URL and the schema, kept apart rather than re-parsed out of the CLI's own + # argument: a test that reads a different schema from the one the command wrote to is a + # test that asserts nothing. + return PostgresStateStore(POSTGRES_URL, schema=self.schema) @pytest.fixture @@ -416,6 +431,21 @@ def test_T278_an_agent_name_containing_a_slash_cannot_be_written(workspace, tree assert _revoked(workspace, tree) == set() +def test_T278_a_trailing_separator_is_a_typed_and_lost_user(workspace, tree): + """`--created-by human-cfo/` is refused rather than read as "every user". + + The widest reading of an ambiguous command is the wrong default during an incident, and + without the guard this exits non-zero anyway through §7.5's empty-selector error, which is + the subsumed shape the mutation table caught twice already. So the message is asserted. + """ + result = workspace.cli("revoke", "--created-by", "human-cfo/") + + assert result.exit_code != 0 + assert "ends with '/'" in result.output + assert "no delegation matched" not in result.output + assert _revoked(workspace, tree) == set() + + # --- T279: --by keeps its 0.7.0 meaning -------------------------------------------------------