fix: allow crystallize to approve own session proposals#49
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds test coverage and documentation for the self-approval bypass fix in crystallize(). Tests verify the guard rejects same-agent approval by default and allows it when review.approver_role is set to trusted-agent in config.yaml. Single-agent crystallization is tested across both JSONL server and sessions modules. CHANGELOG documents the fix. ChangesSelf-approval Guard and Crystallization Fix Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_jsonl_server.py (1)
69-83: ⚡ Quick winAdd explicit same-agent crystallize coverage.
With
VOUCH_AGENTswitched tohuman-reviewerbeforekb.crystallize, this test no longer validates the original regression (approved_by == proposed_byduring crystallize). Please add/adjust a case where the same agent proposes and crystallizes.✅ Minimal test tweak
def test_jsonl_session_lifecycle(store: KBStore, monkeypatch) -> None: src = store.put_source(b"e") monkeypatch.chdir(store.root) + monkeypatch.setenv("VOUCH_AGENT", "session-agent") sess = handle_request({"id": "1", "method": "kb.session_start", "params": {"task": "demo"}}) @@ - monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") cryst = handle_request({"id": "4", "method": "kb.crystallize", "params": {"session_id": sid}})🤖 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 69 - 83, The test currently sets VOUCH_AGENT to "human-reviewer" only before calling kb.crystallize, which misses validating the same-agent crystallize regression; update test_jsonl_session_lifecycle to include a path where the proposer and crystallizer are the same agent by setting monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") before making a propose_claim (or add an additional propose_claim with VOUCH_AGENT set) and then call kb.crystallize while still under that same VOUCH_AGENT to ensure approved_by == proposed_by is exercised for the handle_request calls to kb.propose_claim and kb.crystallize.
🤖 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 `@CHANGELOG.md`:
- Around line 9-10: Update the changelog line to clarify that the new
forbidden-self-approval behavior in proposals.approve() (raising
ProposalError("forbidden_self_approval") when approved_by ==
proposal.proposed_by) is conditional: note that approve() still allows
self-approval when called with _trusted=True (used for session crystallization),
so the entry should state the enforcement applies except for the _trusted
bypass. Reference proposals.approve(), ProposalError("forbidden_self_approval"),
approved_by, proposal.proposed_by, and the _trusted parameter in the revised
wording.
---
Nitpick comments:
In `@tests/test_jsonl_server.py`:
- Around line 69-83: The test currently sets VOUCH_AGENT to "human-reviewer"
only before calling kb.crystallize, which misses validating the same-agent
crystallize regression; update test_jsonl_session_lifecycle to include a path
where the proposer and crystallizer are the same agent by setting
monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") before making a
propose_claim (or add an additional propose_claim with VOUCH_AGENT set) and then
call kb.crystallize while still under that same VOUCH_AGENT to ensure
approved_by == proposed_by is exercised for the handle_request calls to
kb.propose_claim and kb.crystallize.
🪄 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: 8843a7e0-f7a8-40f4-ac08-d44ca87132cf
📒 Files selected for processing (4)
CHANGELOG.mdsrc/vouch/proposals.pysrc/vouch/sessions.pytests/test_jsonl_server.py
|
Thanks for picking this up! The Needs changes
Nice to have
Happy to re-review once #46 lands and this is rebased. Thanks again 🙏 |
Thanks for the detailed review. Working through all the points:
Waiting for #46 to land before rebasing. Will update this PR immediately |
74ca299 to
cbd0442
Compare
Rebased on #46 and reworked the approach as suggested:
104 tests pass. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_sessions.py (1)
50-52: 💤 Low valueRemove redundant imports.
KBStore(line 11) andsess_mod(line 9) are already imported at module level. Only theyamlimport is needed here.♻️ Proposed fix
def test_crystallize_single_agent_succeeds(tmp_path, monkeypatch) -> None: """Single-agent crystallize must succeed when trusted-agent is configured.""" import yaml - from vouch.storage import KBStore - from vouch import sessions as sess_mod store = KBStore.init(tmp_path)Also remove the local re-import of
propose_claimat line 66:- from vouch.proposals import propose_claim propose_claim(🤖 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_sessions.py` around lines 50 - 52, Remove the redundant local imports in tests/test_sessions.py: keep only the yaml import in the shown block and delete the repeated module-level imports of KBStore and sess_mod; also remove the later local re-import of propose_claim so the test uses the module-level propose_claim. Ensure references to KBStore, sess_mod, and propose_claim remain available from the top-level imports already present in the file.
🤖 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_sessions.py`:
- Around line 50-52: Remove the redundant local imports in
tests/test_sessions.py: keep only the yaml import in the shown block and delete
the repeated module-level imports of KBStore and sess_mod; also remove the later
local re-import of propose_claim so the test uses the module-level
propose_claim. Ensure references to KBStore, sess_mod, and propose_claim remain
available from the top-level imports already present in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 614b96e6-6109-4343-803c-b479d8a48652
📒 Files selected for processing (4)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_jsonl_server.pytests/test_sessions.py
|
Fix conflict |
cbd0442 to
1a8a623
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_jsonl_server.py (1)
159-162: ⚡ Quick winAssert crystallize failures are empty in the JSONL lifecycle test.
This test currently only checks
approvedlength; add afailures == []assertion to lock in the no-failure contract for the single-agent trusted flow.Suggested patch
cryst = handle_request({"id": "4", "method": "kb.crystallize", "params": {"session_id": sid}}) + assert cryst["ok"] assert len(cryst["result"]["approved"]) == 1 + assert cryst["result"]["failures"] == []🤖 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 159 - 162, The test currently only asserts that the crystallize response has one approved item; update the JSONL lifecycle test by adding an assertion that failures are empty: after the call to handle_request (which returns cryst), add assert cryst["result"]["failures"] == [] to ensure the single-agent trusted flow reports no failures.
🤖 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 `@CHANGELOG.md`:
- Line 11: Update the CHANGELOG entry to make clear that the bypass is enabled
by an explicit configuration opt-in (setting the system/config flag to
"trusted-agent") rather than being automatically set by crystallize(); state
that crystallize() still follows the normal approve() rules and only operates
with review.approver_role: trusted-agent when that config is explicitly
provided. Replace the current line referencing crystallize() setting
review.approver_role with wording that the config opt-in enables trusted-agent
behavior and crystallize() then proceeds under standard approve() logic.
---
Nitpick comments:
In `@tests/test_jsonl_server.py`:
- Around line 159-162: The test currently only asserts that the crystallize
response has one approved item; update the JSONL lifecycle test by adding an
assertion that failures are empty: after the call to handle_request (which
returns cryst), add assert cryst["result"]["failures"] == [] to ensure the
single-agent trusted flow reports no failures.
🪄 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: 5ffe1228-0195-4ae9-a84f-57d85c0573e1
📒 Files selected for processing (4)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_jsonl_server.pytests/test_sessions.py
|
|
||
| ### Fixed | ||
| - Raise `ProposalError("forbidden_self_approval")` in `proposals.approve()` when `approved_by == proposal.proposed_by`, enforcing the review-gate guarantee documented in the README and CONTRIBUTING. | ||
| - `crystallize()` now sets `review.approver_role: trusted-agent` context so single-agent sessions can be crystallized without hitting the `forbidden_self_approval` guard (#47). |
There was a problem hiding this comment.
Clarify that trusted-agent is an explicit config opt-in, not an automatic crystallize() side effect.
Current wording implies crystallize() sets review.approver_role itself. Based on this PR’s behavior, the bypass is enabled when config is set to trusted-agent, and crystallize() then proceeds under normal approve() rules.
✏️ Suggested wording
-- `crystallize()` now sets `review.approver_role: trusted-agent` context so single-agent sessions can be crystallized without hitting the `forbidden_self_approval` guard (`#47`).
+- Single-agent `crystallize()` can proceed without `forbidden_self_approval` when `config.yaml` sets `review.approver_role: trusted-agent` (`#47`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `crystallize()` now sets `review.approver_role: trusted-agent` context so single-agent sessions can be crystallized without hitting the `forbidden_self_approval` guard (#47). | |
| - Single-agent `crystallize()` can proceed without `forbidden_self_approval` when `config.yaml` sets `review.approver_role: trusted-agent` (`#47`). |
🤖 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 `@CHANGELOG.md` at line 11, Update the CHANGELOG entry to make clear that the
bypass is enabled by an explicit configuration opt-in (setting the system/config
flag to "trusted-agent") rather than being automatically set by crystallize();
state that crystallize() still follows the normal approve() rules and only
operates with review.approver_role: trusted-agent when that config is explicitly
provided. Replace the current line referencing crystallize() setting
review.approver_role with wording that the config opt-in enables trusted-agent
behavior and crystallize() then proceeds under standard approve() logic.
|
Fix CI |
… instead Now that approve() supports the review.approver_role: trusted-agent opt-out, crystallize() no longer needs a private _trusted bypass. Single-agent setups configure the opt-out explicitly; crystallize() calls approve() with no special flag. Also: - Add test_crystallize_single_agent_succeeds in test_sessions.py - Update crystallize JSONL test to exercise the real single-agent scenario instead of switching to human-reviewer Fixes vouchdev#47
|
@Tet-9 Any update here? |
Yeah, sure,..... Working on it |
1a8a623 to
199c7cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_sessions.py`:
- Around line 50-52: The import block in tests/test_sessions.py is flagged by
Ruff I001 for incorrect grouping/sorting; reorder the imports into three
standard groups (stdlib, third-party, local) and sort them alphabetically within
each group — e.g., place "import yaml" in the third-party section and "from
vouch.storage import KBStore" and "from vouch import sessions as sess_mod" in
the local/package section, and apply the same ordering/sorting fix to the other
import block around lines 78-81 so Ruff no longer reports I001.
🪄 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: 612d066a-f3a9-4f4d-bdb1-dab68473ec37
📒 Files selected for processing (3)
CHANGELOG.mdtests/test_jsonl_server.pytests/test_sessions.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
|
@plind-junior , fixed man🫡 |
|
I guess it's good to go now 😄 |
|
LGTM! |
2 participants
Problem
sessions.crystallize()callsapprove(..., approved_by=approver)where
approveris_agent()— the same value recorded asproposed_byon every proposal filed during the session. Sinceapprove()enforces theforbidden_self_approvalguard, everyproposal hits the guard and is dumped into
failures[]. Single-agentcrystallize silently approves nothing and returns an empty
approvedlist with all proposals in
failures.Fix
Drop the
_trustedbypass entirely. Now thatapprove()supportsreview.approver_role: trusted-agentas an explicit opt-out (landedin #46), single-agent setups configure that key in
config.yamlandcrystallize()callsapprove()with no special flag — keeping thepublic API clean.
Tests
test_crystallize_single_agent_succeedsintest_sessions.py:starts a session as
alice, files a proposal asalice, setsreview.approver_role: trusted-agent, callscrystallize(approver="alice"),asserts
approvedis non-empty andfailuresis emptyscenario instead of switching to
human-reviewer104 tests pass, 4 deselected.
Fixes #47
Summary by CodeRabbit
Release Notes
Bug Fixes
review.approver_roleis configured astrusted-agent, removing the previous self-approval restriction for this valid workflow.Tests