From cedd6c9002d0de1dcaece3237e413ab5d198fb1b Mon Sep 17 00:00:00 2001 From: dalledajay-coder Date: Thu, 9 Jul 2026 22:27:14 +0000 Subject: [PATCH] fix(delete): block self-approval of protected-page deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the protected-kind gate in _approval_block_reason only covered PAGE proposals, so under review.approver_role: trusted-agent a proposer could file and self-approve a DELETE targeting a protected page — removing a policy-bearing page it could not have edited without a second reviewer. extend the gate to DELETE proposals that target a page: resolve the page kind from the live artifact (what approve would actually remove), falling back to the propose-time snapshot so the guard also holds on the idempotent already-gone path. check_approvable shares the helper, so the batch precheck reports the same reason. unprotected deletes keep the trusted-agent opt-out unchanged. --- CHANGELOG.md | 7 ++++++ src/vouch/proposals.py | 29 +++++++++++++++++++++++ tests/test_delete.py | 53 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb2ec563..287b0691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ All notable changes to vouch are documented here. Format follows - console Dashboard view: 12-month activity calendar, last-30-days bars, hour-of-week heatmap, top actors and event mix, driven by `kb.activity`. +### Fixed +- delete proposals targeting a protected page kind could be self-approved + under `review.approver_role: trusted-agent` — the protected-kind gate only + covered page edits, so a proposer could remove a policy-bearing page it + could not have changed on its own. the gate now covers the delete path + (live page kind first, propose-time snapshot on the already-gone path). + ## [1.2.2] — 2026-07-07 ### Packaging diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index e66c1298..24052add 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -389,6 +389,17 @@ def _approval_block_reason( f"forbidden_self_approval: page kind '{page_type}' is protected — " "it always requires a reviewer other than the proposer" ) + # The same invariant covers DELETE proposals that target a page: + # removing a policy-bearing page is at least as sensitive as editing + # it, so the trusted-agent opt-out must not widen to protected kinds + # through the delete path either. + if proposal.kind == ProposalKind.DELETE: + page_type = _delete_target_page_type(store, proposal.payload) + if page_type and load_page_kind_registry(store).is_protected(page_type): + return ( + f"forbidden_self_approval: page kind '{page_type}' is protected — " + "deleting it always requires a reviewer other than the proposer" + ) cfg: dict[str, Any] = {} try: loaded = yaml.safe_load((store.kb_dir / "config.yaml").read_text(encoding="utf-8")) @@ -408,6 +419,24 @@ def _approval_block_reason( return None +def _delete_target_page_type(store: KBStore, payload: dict[str, Any]) -> str: + """Page kind a DELETE proposal targets, or '' when it is not a page. + + Prefers the live page (that is what approve would remove); falls back to + the propose-time snapshot so the protected-kind gate still holds on the + idempotent already-gone path. + """ + if payload.get("target_kind") != "page": + return "" + try: + return store.get_page(str(payload.get("id", ""))).type + except ArtifactNotFoundError: + snapshot = payload.get("snapshot") + if isinstance(snapshot, dict): + return str(snapshot.get("type", "")) + return "" + + def _payload_block_reason(store: KBStore, proposal: Proposal) -> str | None: """Dry-run the put_*-side ref guards, return reason string or None. diff --git a/tests/test_delete.py b/tests/test_delete.py index 31b5d7de..8132ca42 100644 --- a/tests/test_delete.py +++ b/tests/test_delete.py @@ -5,6 +5,7 @@ from pathlib import Path import pytest +import yaml from vouch import audit, capabilities, index_db from vouch.jsonl_server import handle_request @@ -313,6 +314,58 @@ def test_delete_forbids_self_approval(store: KBStore) -> None: approve(store, pr.id, approved_by="same") +def _trusted_agent_with_protected_voice(store: KBStore) -> None: + cfg = yaml.safe_load(store.config_path.read_text()) + cfg["review"] = {"approver_role": "trusted-agent"} + cfg["page_kinds"] = {"voice": {"protected": True}} + store.config_path.write_text(yaml.safe_dump(cfg)) + + +def test_delete_protected_page_blocks_self_approval_despite_trusted_agent( + store: KBStore, +) -> None: + """Deleting a protected page must need a distinct reviewer, exactly like + editing one — otherwise the trusted-agent opt-out lets a proposer remove + a policy-bearing page that it could not have changed on its own.""" + _trusted_agent_with_protected_voice(store) + store.put_page(Page(id="p1", title="email voice", body="b", type="voice")) + pr = propose_delete(store, target_kind="page", target_id="p1", proposed_by="agent") + reason = check_approvable(store, pr.id, approved_by="agent") + assert reason is not None and "protected" in reason + with pytest.raises(ProposalError, match="protected"): + approve(store, pr.id, approved_by="agent") + # the page survives and the proposal stays pending + assert store._page_path("p1").exists() + assert any(p.id == pr.id for p in store.list_proposals(ProposalStatus.PENDING)) + # a distinct reviewer can still approve the delete + approve(store, pr.id, approved_by="reviewer") + assert not store._page_path("p1").exists() + + +def test_delete_protected_page_guard_holds_when_target_already_gone( + store: KBStore, +) -> None: + """The idempotent already-gone path finalizes via the snapshot, so the + protected-kind gate must read the kind from the snapshot too.""" + _trusted_agent_with_protected_voice(store) + store.put_page(Page(id="p1", title="email voice", body="b", type="voice")) + pr = propose_delete(store, target_kind="page", target_id="p1", proposed_by="agent") + store.delete_page("p1") # crash-retry: file already removed + with pytest.raises(ProposalError, match="protected"): + approve(store, pr.id, approved_by="agent") + # a distinct reviewer still finalizes the idempotent approve + result = approve(store, pr.id, approved_by="reviewer") + assert result.id == "p1" + + +def test_delete_unprotected_page_keeps_trusted_agent_opt_out(store: KBStore) -> None: + _trusted_agent_with_protected_voice(store) + store.put_page(Page(id="p2", title="scratch notes", body="b")) + pr = propose_delete(store, target_kind="page", target_id="p2", proposed_by="agent") + approve(store, pr.id, approved_by="agent") + assert not store._page_path("p2").exists() + + def test_check_approvable_flags_referenced_delete(store: KBStore) -> None: _claim(store, "c1") pr = propose_delete(store, target_kind="claim", target_id="c1", proposed_by="agent")