Skip to content

fix(delete): block self-approval of protected-page deletes#448

Open
dalledajay-coder wants to merge 1 commit into
vouchdev:testfrom
dalledajay-coder:fix/protected-page-delete-self-approval
Open

fix(delete): block self-approval of protected-page deletes#448
dalledajay-coder wants to merge 1 commit into
vouchdev:testfrom
dalledajay-coder:fix/protected-page-delete-self-approval

Conversation

@dalledajay-coder

Copy link
Copy Markdown

What changed

_approval_block_reason now applies the protected-page-kind gate to DELETE proposals that target a page, not just to PAGE proposals. the page kind is resolved 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.

Why

with review.approver_role: trusted-agent and a protected page kind (say voice), editing that page correctly refuses self-approval — "it always requires a reviewer other than the proposer". but the new delete path didn't run that check, so the same proposer could file kb.propose_delete for the page and approve it themselves:

edit_pr = propose_page(store, title="email voice", body="b", page_type="voice", proposed_by="agent")
approve(store, edit_pr.id, approved_by="agent")   # ProposalError: protected
del_pr = propose_delete(store, target_kind="page", target_id="email-voice", proposed_by="agent")
approve(store, del_pr.id, approved_by="agent")    # succeeded — page gone

removing a policy-bearing page is at least as sensitive as editing it, and the comment on the existing gate says the opt-out "can never widen it" — the delete path widened it. deletes are also irreversible (reversible=False in the audit event), so this was the one write a trusted agent could make to protected knowledge without a second reviewer.

no .vouch/ directory migration concerns: the change only tightens an approve-time check; nothing on disk changes shape.

Testing

  • tests/test_delete.py::test_delete_protected_page_blocks_self_approval_despite_trusted_agent — fails on current test (the approve succeeds and the page is unlinked), passes with the fix; also asserts a distinct reviewer can still approve.
  • tests/test_delete.py::test_delete_protected_page_guard_holds_when_target_already_gone — covers the snapshot fallback on the crash-retry path; fails before, passes after.
  • tests/test_delete.py::test_delete_unprotected_page_keeps_trusted_agent_opt_out — guards against over-blocking; passes before and after.

ran locally:

python -m ruff check src tests   # all checks passed
python -m mypy src               # success: no issues found in 91 source files
python -m pytest tests/ -q --ignore=tests/embeddings   # 1153 passed, 28 skipped

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6a50707a-acca-4e69-a74c-75260432e5b3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 9, 2026
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.
@dalledajay-coder dalledajay-coder force-pushed the fix/protected-page-delete-self-approval branch from 2eb47d8 to cedd6c9 Compare July 10, 2026 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs documentation, specs, examples, and repo guidance size: S 50-199 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant