fix: enforce forbidden_self_approval gate in proposals.approve() - #46
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR forbids self-approval in proposals.approve() by raising ProposalError("forbidden_self_approval") when the approver equals the proposer, updates JSONL tests to set VOUCH_AGENT="human-reviewer", and adds a changelog entry documenting the fix. ChangesSelf-Approval Guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_jsonl_server.py (1)
60-60: ⚡ Quick winConsider adding an explicit test for self-approval rejection.
The updated tests ensure that approval with a different agent succeeds, but there's no explicit test case that verifies self-approval is rejected with
ProposalError("forbidden_self_approval"). This creates a test coverage gap for the core behavior added in this PR.🧪 Suggested test case for self-approval rejection
Add a new test to explicitly verify self-approval is forbidden:
def test_jsonl_self_approval_forbidden(store: KBStore, monkeypatch) -> None: src = store.put_source(b"evidence") monkeypatch.chdir(store.root) pr = handle_request({"id": "1", "method": "kb.propose_claim", "params": {"text": "test", "evidence": [src.id]}}) pid = pr["result"]["proposal_id"] # Attempt to approve with same agent (should fail) resp = handle_request({"id": "2", "method": "kb.approve", "params": {"proposal_id": pid}}) assert not resp["ok"] assert "forbidden_self_approval" in resp["error"]["message"]Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jsonl_server.py` at line 60, Add a new test function (e.g., test_jsonl_self_approval_forbidden) that uses the existing store and monkeypatch fixtures, creates an evidence source via store.put_source, chdirs into store.root, submits a proposal via handle_request({"method":"kb.propose_claim", ...}) and captures the proposal_id, then attempts to approve the proposal using handle_request({"method":"kb.approve", "params":{"proposal_id": pid}}) without changing VOUCH_AGENT so the approver is the same agent; assert the response is not ok and that resp["error"]["message"] contains "forbidden_self_approval" to explicitly verify self-approval is rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_jsonl_server.py`:
- Line 60: Add a new test function (e.g., test_jsonl_self_approval_forbidden)
that uses the existing store and monkeypatch fixtures, creates an evidence
source via store.put_source, chdirs into store.root, submits a proposal via
handle_request({"method":"kb.propose_claim", ...}) and captures the proposal_id,
then attempts to approve the proposal using
handle_request({"method":"kb.approve", "params":{"proposal_id": pid}}) without
changing VOUCH_AGENT so the approver is the same agent; assert the response is
not ok and that resp["error"]["message"] contains "forbidden_self_approval" to
explicitly verify self-approval is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2faf6044-d9cd-4136-b748-194e35dad55c
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_jsonl_server.py
|
Thanks for picking this up — the core check is in the right place. A few things need to be addressed before this can merge: Blocking
Non-blocking
Happy to re-review once the changelog marker, the opt-out, and the tests are in. Thanks again for the contribution! 🙏 |
Updated — all blocking points addressed:
All 103 tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_jsonl_server.py (1)
54-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the actor identity explicit from the first request in these tests.
These tests still inherit ambient
VOUCH_AGENT. If the test runner already exportsVOUCH_AGENT=human-reviewer,test_jsonl_full_flow()andtest_jsonl_session_lifecycle()will propose as the same actor they later use to approve, so they fail spuriously. The self-approval tests are also relying on that default implicitly. Set or clearVOUCH_AGENTbefore the first request in each test so proposer/approver identity is deterministic.Suggested fix
def test_jsonl_full_flow(store: KBStore, monkeypatch) -> None: src = store.put_source(b"raw evidence") monkeypatch.chdir(store.root) + monkeypatch.setenv("VOUCH_AGENT", "proposal-agent") pr = handle_request({"id": "1", "method": "kb.propose_claim", "params": {"text": "JWT used", "evidence": [src.id]}}) pid = pr["result"]["proposal_id"] monkeypatch.setenv("VOUCH_AGENT", "human-reviewer")def test_jsonl_session_lifecycle(store: KBStore, monkeypatch) -> None: src = store.put_source(b"e") monkeypatch.chdir(store.root) + monkeypatch.setenv("VOUCH_AGENT", "proposal-agent") sess = handle_request({"id": "1", "method": "kb.session_start", "params": {"task": "demo"}})def test_jsonl_self_approval_forbidden(store: KBStore, monkeypatch) -> None: src = store.put_source(b"evidence") monkeypatch.chdir(store.root) + monkeypatch.setenv("VOUCH_AGENT", "same-agent") pr = handle_request({"id": "1", "method": "kb.propose_claim", "params": {"text": "test claim", "evidence": [src.id]}})def test_jsonl_self_approval_allowed_with_trusted_agent_config( store: KBStore, monkeypatch ) -> None: @@ src = store.put_source(b"evidence") monkeypatch.chdir(store.root) + monkeypatch.setenv("VOUCH_AGENT", "same-agent") pr = handle_request({"id": "1", "method": "kb.propose_claim", "params": {"text": "test claim", "evidence": [src.id]}})Also applies to: 140-152, 157-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_jsonl_server.py` around lines 54 - 60, The tests (e.g., test_jsonl_full_flow and test_jsonl_session_lifecycle) rely on ambient VOUCH_AGENT; ensure the actor identity is deterministic by explicitly setting or clearing VOUCH_AGENT before the first call to handle_request/propose (move or add monkeypatch.setenv("VOUCH_AGENT", "<actor>") or monkeypatch.delenv("VOUCH_AGENT", raising=False) at the top of each test before the initial handle_request call), and update the other test blocks (lines ~140-152 and ~157-188) similarly so proposer and approver identities cannot inherit the external environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/proposals.py`:
- Around line 231-241: The loaded YAML result from yaml.safe_load may be a
scalar or list, so normalize it to a dict before using chained .get() calls:
after cfg = yaml.safe_load(...) or {} ensure cfg is a dict (e.g., if not
isinstance(cfg, dict): cfg = {}) and likewise guard the nested review lookup
(ensure review = cfg.get("review") is a dict before checking
review.get("approver_role")), then keep the existing ProposalError check that
raises when approver_role != "trusted-agent". Target the cfg variable, the
yaml.safe_load call, and the review.approver_role check in this function to make
the validation fail-closed.
---
Outside diff comments:
In `@tests/test_jsonl_server.py`:
- Around line 54-60: The tests (e.g., test_jsonl_full_flow and
test_jsonl_session_lifecycle) rely on ambient VOUCH_AGENT; ensure the actor
identity is deterministic by explicitly setting or clearing VOUCH_AGENT before
the first call to handle_request/propose (move or add
monkeypatch.setenv("VOUCH_AGENT", "<actor>") or
monkeypatch.delenv("VOUCH_AGENT", raising=False) at the top of each test before
the initial handle_request call), and update the other test blocks (lines
~140-152 and ~157-188) similarly so proposer and approver identities cannot
inherit the external environment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4723c08-1ff3-44dc-b541-9cc1d83ded0e
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_jsonl_server.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
|
Fix conflict |
The README states that writes require approval and the review gate
prevents the same actor from both proposing and approving. In practice
proposals.approve() only checked proposal status — no guard compared
approved_by to proposal.proposed_by. Any agent could propose and
immediately approve its own claims, bypassing the review gate entirely.
Raise ProposalError('forbidden_self_approval') when approved_by
matches proposal.proposed_by. Update two JSONL server tests that
used the same VOUCH_AGENT for both propose and approve to set a
distinct human-reviewer identity before the approve call.
Fixes vouchdev#45
- Remove stray '=======' conflict marker from CHANGELOG.md - Add review.approver_role: trusted-agent opt-out in approve() so single-agent and fully-automated KBs can self-approve when explicitly configured - Improve error message with remediation hint pointing to config option - Add test_jsonl_self_approval_forbidden: asserts ProposalError is raised when approved_by == proposed_by - Add test_jsonl_self_approval_allowed_with_trusted_agent_config: asserts self-approval succeeds when opt-out is configured Fixes vouchdev#45
b4ce197 to
9d2fce5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/vouch/proposals.py (1)
231-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix mypy error and normalize loaded config to dict.
The pipeline fails because
cfglacks a type annotation. Additionally,yaml.safe_load()can return a scalar or list for valid-but-malformed YAML, which would causeAttributeErroron the chained.get()calls at line 237 instead of failing closed withProposalError.Proposed fix addressing both issues
if approved_by == proposal.proposed_by: - cfg = {} + cfg: dict[str, Any] = {} try: import yaml - cfg = yaml.safe_load((store.kb_dir / "config.yaml").read_text()) or {} + loaded = yaml.safe_load((store.kb_dir / "config.yaml").read_text()) + if isinstance(loaded, dict): + cfg = loaded except Exception: pass - if cfg.get("review", {}).get("approver_role") != "trusted-agent": + review_cfg = cfg.get("review") + approver_role = ( + review_cfg.get("approver_role") if isinstance(review_cfg, dict) else None + ) + if approver_role != "trusted-agent": raise ProposalError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/proposals.py` around lines 231 - 241, Annotate cfg as a mapping (e.g., cfg: Dict[str, Any]) to satisfy mypy and after calling yaml.safe_load((store.kb_dir / "config.yaml").read_text()) normalize its result by checking isinstance(loaded, dict) and assigning cfg = loaded if so, otherwise cfg = {}; keep the broad try/except around the import/load as-is so malformed YAML or load errors fall through to the safe default and the subsequent cfg.get(...) checks remain valid (refer to the cfg variable and the safe_load call in proposals.py that precedes the ProposalError).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/vouch/proposals.py`:
- Around line 231-241: Annotate cfg as a mapping (e.g., cfg: Dict[str, Any]) to
satisfy mypy and after calling yaml.safe_load((store.kb_dir /
"config.yaml").read_text()) normalize its result by checking isinstance(loaded,
dict) and assigning cfg = loaded if so, otherwise cfg = {}; keep the broad
try/except around the import/load as-is so malformed YAML or load errors fall
through to the safe default and the subsequent cfg.get(...) checks remain valid
(refer to the cfg variable and the safe_load call in proposals.py that precedes
the ProposalError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad5b80d9-f2ae-464d-b22a-aba9f4af531b
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_jsonl_server.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
All 128 tests pass locally with the exact CI config. The CI runs are |
Add type annotation cfg: dict[str, Any] to satisfy mypy. Normalize yaml.safe_load result with isinstance check so malformed YAML or non-dict values fall through to the safe default instead of causing AttributeError on .get() calls. Suggested by CodeRabbit review.
|
@plind-junior review time boss 🫡 |
Applied — annotated |
|
@plind-junior ? |
|
@plind-junior , the conflicts are already fixed, can you review it now?? |
|
With the 3 others too |
Fixes #45
proposals.approve()did not check whetherapproved_bymatchesproposal.proposed_by, allowing an agent to self-approve its ownproposals and bypass the review gate entirely.
Enforce the
forbidden_self_approvalguard: raiseProposalErrorwhenapproved_by == proposal.proposed_by, unlessreview.approver_roleisset to
trusted-agentin the KB config.Adds tests covering both the blocked and opted-out cases.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests