fix(lifecycle): reject supersede/contradict on already-retired claims#539
Closed
rsnetworkinginc wants to merge 1 commit into
Closed
fix(lifecycle): reject supersede/contradict on already-retired claims#539rsnetworkinginc wants to merge 1 commit into
rsnetworkinginc wants to merge 1 commit into
Conversation
supersede(old, new) never checked new's status, so supersede(a, b) followed by supersede(b, a) silently closed a 2-cycle: both claims ended up status=superseded, each pointing at the other as its successor. context._RETRACTED_CLAIM_STATUSES then excludes SUPERSEDED from every retrieval surface, so both claims vanish with no live head - knowledge lost without anyone asking to delete it. separately, contradict(a, b) unconditionally set both sides to CONTESTED, which is not in the retracted-status set. contradicting an already-superseded/archived/redacted claim against anything else silently un-retired it back into live retrieval, making the supersede/archive/redact controls decorative. both functions now raise LifecycleError when a claim on the "becomes live" side of the call is already retired, mirroring the existing self-reference guards. validation: python -m pytest tests/ -q --ignore=tests/embeddings python -m mypy src python -m ruff check src tests all green. new regression tests confirmed red pre-fix / green post-fix via git stash.
github-actions
Bot
added
docs
WalkthroughChangesLifecycle guards
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 2
🤖 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_storage.py`:
- Around line 863-913: The rejection tests in
test_supersede_rejects_archived_new_claim and
test_contradict_rejects_retired_claim only verify one status, not complete
persistence immutability. Snapshot both involved claims and their lifecycle
relations before each expected LifecycleError, then compare all persisted fields
and relations afterward to confirm the rejected operation leaves on-disk state
unchanged.
- Around line 863-913: The lifecycle tests
test_supersede_rejects_retired_new_claim,
test_supersede_rejects_archived_new_claim, and
test_contradict_rejects_retired_claim are in the wrong module. Move these test
functions and their required imports/fixtures to tests/test_lifecycle.py,
removing them from tests/test_storage.py while preserving their behavior and
assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39cb40d9-bda9-4cc6-91b6-ec640f73a851
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/lifecycle.pytests/test_storage.py
Comment on lines
+863
to
+913
| def test_supersede_rejects_retired_new_claim(store: KBStore) -> None: | ||
| # Guards against a 2-cycle: A superseded_by B, then B "superseded_by" A. | ||
| # Before the fix this silently corrupted both claims to | ||
| # status=superseded, pointing at each other — retrieval excludes | ||
| # SUPERSEDED, so both claims vanished from every retrieval surface with | ||
| # no live successor. | ||
| src = store.put_source(b"e") | ||
| store.put_claim(Claim(id="a", text="a", evidence=[src.id])) | ||
| store.put_claim(Claim(id="b", text="b", evidence=[src.id])) | ||
| lifecycle.supersede(store, old_claim_id="a", new_claim_id="b", actor="u") | ||
| with pytest.raises(lifecycle.LifecycleError, match="already superseded"): | ||
| lifecycle.supersede(store, old_claim_id="b", new_claim_id="a", actor="u") | ||
| # Neither claim's state was touched by the rejected call. | ||
| a = store.get_claim("a") | ||
| b = store.get_claim("b") | ||
| assert a.status == ClaimStatus.SUPERSEDED | ||
| assert a.superseded_by == "b" | ||
| assert a.supersedes == [] | ||
| assert b.status != ClaimStatus.SUPERSEDED | ||
| assert b.superseded_by is None | ||
|
|
||
|
|
||
| def test_supersede_rejects_archived_new_claim(store: KBStore) -> None: | ||
| src = store.put_source(b"e") | ||
| store.put_claim(Claim(id="old", text="old", evidence=[src.id])) | ||
| store.put_claim(Claim(id="new", text="new", evidence=[src.id])) | ||
| lifecycle.archive(store, claim_id="new", actor="u") | ||
| with pytest.raises(lifecycle.LifecycleError, match="already archived"): | ||
| lifecycle.supersede(store, old_claim_id="old", new_claim_id="new", actor="u") | ||
| assert store.get_claim("old").status != ClaimStatus.SUPERSEDED | ||
|
|
||
|
|
||
| def test_contradict_rejects_retired_claim(store: KBStore) -> None: | ||
| # A retired (superseded/archived/redacted) claim must not be revivable | ||
| # into CONTESTED — CONTESTED is not in the retracted-status set, so | ||
| # before the fix this silently un-retired the claim back into every | ||
| # retrieval surface (context.py's _RETRACTED_CLAIM_STATUSES exclusion). | ||
| src = store.put_source(b"e") | ||
| store.put_claim(Claim(id="old", text="old", evidence=[src.id])) | ||
| store.put_claim(Claim(id="new", text="new", evidence=[src.id])) | ||
| store.put_claim(Claim(id="c", text="c", evidence=[src.id])) | ||
| lifecycle.supersede(store, old_claim_id="old", new_claim_id="new", actor="u") | ||
| with pytest.raises(lifecycle.LifecycleError, match="already superseded"): | ||
| lifecycle.contradict(store, claim_a="old", claim_b="c", actor="u") | ||
| old = store.get_claim("old") | ||
| c = store.get_claim("c") | ||
| assert old.status == ClaimStatus.SUPERSEDED | ||
| assert old.contradicts == [] | ||
| assert c.status != ClaimStatus.CONTESTED | ||
| assert c.contradicts == [] | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
assert complete persisted-state immutability on rejection.
The archived case only checks that old was not superseded. Snapshot both claims and relations before each rejected call, then compare their persisted values afterward; this protects against future partial writes.
As per path instructions, “add/adjust regression tests that assert the correct failure mode ... and that no on-disk lifecycle state is mutated on failure.”
🤖 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_storage.py` around lines 863 - 913, The rejection tests in
test_supersede_rejects_archived_new_claim and
test_contradict_rejects_retired_claim only verify one status, not complete
persistence immutability. Snapshot both involved claims and their lifecycle
relations before each expected LifecycleError, then compare all persisted fields
and relations afterward to confirm the rejected operation leaves on-disk state
unchanged.
Source: Path instructions
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
move these lifecycle tests to the matching test module.
These tests cover src/vouch/lifecycle.py, so they belong in tests/test_lifecycle.py, not tests/test_storage.py.
As per coding guidelines, “Tests must mirror module names using the strict tests/test_<module>.py convention.”
🤖 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_storage.py` around lines 863 - 913, The lifecycle tests
test_supersede_rejects_retired_new_claim,
test_supersede_rejects_archived_new_claim, and
test_contradict_rejects_retired_claim are in the wrong module. Move these test
functions and their required imports/fixtures to tests/test_lifecycle.py,
removing them from tests/test_storage.py while preserving their behavior and
assertions.
Source: Coding guidelines
|
closing automatically: CodeRabbit requested changes and this pr has had no new commits for 2 days. the feedback still stands — push a fix and reopen this pr (or open a fresh one) and it will be reviewed again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
lifecycle.supersede()andlifecycle.contradict()never checked the current status of the claim on the "becomes live" side of the call, which let two related inconsistent states land on disk:supersede(old, new)never checkednew's status. Callingsupersede(a, b)and thensupersede(b, a)silently closes a 2-cycle — both claims end upstatus: superseded, each pointing at the other viasuperseded_by/supersedes.context._RETRACTED_CLAIM_STATUSESexcludesSUPERSEDEDfrom every retrieval surface, so both claims vanish fromkb.context/kb.search/recall with no live successor — knowledge lost without anyone asking to delete it.contradict(a, b)unconditionally sets both sides toCONTESTED, which is not in the retracted-status set (contested claims are still part of the conversation, just disputed). Contradicting an already-superseded/archived/redacted claim against anything else silently un-retires it straight back into live retrieval, making the supersede/archive/redact controls decorative.Reproduced by hand against a scratch
.vouch/(not from a filed issue — found by exercising the CLI directly, same approach as #509).what changed
src/vouch/lifecycle.py:supersede(): raisesLifecycleErrorifnew's current status is already superseded/archived/redacted, mirroring the existingold_claim_id == new_claim_idself-reference guard.contradict(): raisesLifecycleErrorif either side's current status is already superseded/archived/redacted, mirroring the self-reference guard added in fix(lifecycle): reject claim self-contradiction #437.No object-model,
kb.*surface, on-disk layout, or audit-log shape change — both are existing lifecycle functions, this only tightens their pre-write validation (same shape as #437's self-contradiction fix).Nothing breaks for an existing
.vouch/directory: pre-existing (already-corrupted) data on disk is untouched; the guard only blocks new calls that would create the inconsistent state going forward.test plan
New regression tests in
tests/test_storage.py:test_supersede_rejects_retired_new_claim— reproduces the 2-cycle, assertsLifecycleErrorand that neither claim's state changed.test_supersede_rejects_archived_new_claimtest_contradict_rejects_retired_claimConfirmed each fails on the pre-fix code (
git stashthelifecycle.pychange) and passes after.All green.
changelog
Added a
### Fixedentry under## [Unreleased]inCHANGELOG.md.Summary by CodeRabbit
Bug Fixes
Documentation