fix(storage): reject dangling Claim graph references on every write path#201
fix(storage): reject dangling Claim graph references on every write path#201galuis116 wants to merge 3 commits into
Conversation
Claim's four graph-reference fields — entities, supersedes, superseded_by, contradicts — were validated by no write path. put_claim checked only claim.evidence; update_claim re-validated the model (no KB access, so no ref check); bundle.import_apply writes claim YAML straight to disk. The vouchdev#124 graph-integrity fix closed Relation.source/target/evidence and Page.entities/sources but, as its own storage.py comment shows, skipped the Claim's own reference fields — even though fsck already declares dangling_supersedes / dangling_superseded_by / dangling_contradicts as error-severity findings. The invariant was articulated but enforced by no writer (same shape as vouchdev#81 / vouchdev#123). - storage.KBStore._validate_claim_refs: entities -> entity ids, supersedes/contradicts/superseded_by -> claim ids. Called from both put_claim and update_claim (the latter closes the in-place-mutation reach path, mirroring the vouchdev#82 re-validation fix). - bundle.import_check: extend the claim branch of the graph-integrity pass with the same checks, matching the existing page-ref checks, so a bundle can't land a dangling claim ref through import_apply's direct write. - Honest lifecycle writes are unaffected: supersede/contradict load both ends via get_claim before linking, so their refs always resolve. Regression tests in test_storage.py, test_bundle.py, and test_health.py; the fsck/CLI dangling-chain tests now write poisoned YAML directly to disk to reproduce the legacy on-disk state fsck must still surface. Closes vouchdev#196.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
The `test` merge restored this test to the put_claim(Claim(supersedes= ["ghost"])) form, which now trips the new _validate_claim_refs guard and fails CI. Write the dangling-ref claim YAML directly to disk instead — the same approach already used by the fsck dangling tests in test_health.py — so it reproduces the legacy/poisoned on-disk state fsck must still surface, without relying on a write path that now (correctly) rejects it.
galuis116
force-pushed
the
fix/claim-dangling-graph-refs
branch
from
c82a5f7 to
d3a134b
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3a134bdbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise ValueError( | ||
| f"claim {claim.id} cites unknown source/evidence {cid_or_sid}" | ||
| ) | ||
| self._validate_claim_refs(claim) |
There was a problem hiding this comment.
With this new write-time guard, a claim proposal whose entities payload contains a typo (possible through MCP/JSONL kb.propose_claim, since propose_claim only validates evidence) now fails only when approve() reaches store.put_claim. check_approvable() still returns None for that proposal, so the default vouch approve a b path passes the all-or-nothing precheck and then catches this ValueError while continuing to approve later IDs, contradicting the documented “nothing was approved” semantics. Please validate claim entity refs before filing/prechecking proposals, or include this guard in check_approvable().
Useful? React with 👍 / 👎.
| for eid in claim.entities: | ||
| if not self._entity_path(eid).exists(): | ||
| raise ValueError( |
There was a problem hiding this comment.
When this starts rejecting claim.entities, any existing KB that already has a claim pointing at a deleted or misspelled entity can no longer archive/confirm/update that claim, but vouch fsck still only checks the claim lifecycle fields (supersedes, superseded_by, contradicts) and never reports dangling claim entities. Please add a lint/fsck finding for these entity refs alongside the new write-time gate so users have a preflight repair path instead of discovering the blocker only when an unrelated update fails.
Useful? React with 👍 / 👎.
| # dangling entities/supersedes/superseded_by/contradicts link that the | ||
| # model validator can't catch (it has no KB access). Mirrors the | ||
| # put_claim guard so the update path can't reintroduce the gap. | ||
| self._validate_claim_refs(claim) |
There was a problem hiding this comment.
Because update_claim() now raises on any existing dangling graph ref, lifecycle operations can leave a half-applied state when the second claim is legacy/poisoned. For example, vouch supersede old new writes old.status = superseded first, then store.update_claim(new) raises if new.entities or another graph field points at a missing artifact, so the KB records old.superseded_by = new without the reciprocal new.supersedes, relation, or audit event. Please pre-validate all touched claims before the first write or make the lifecycle update atomic.
Useful? React with 👍 / 👎.
2 participants
What changed
Adds a storage-layer reference guard,
KBStore._validate_claim_refs(src/vouch/storage.py), that rejects a Claim whoseentities,supersedes,superseded_by, orcontradictspoint at an artifact not present in the KB. It's called from bothput_claim(alongside the existingevidenceloop) andupdate_claim(alongside the existingClaim.model_validatere-check).bundle.import_checkgains the matching check in theclaimbranch of its graph-integrity pass, mirroring thepagechecks already there. No model, on-disk-layout, schema, bundle-format, MCP/JSONL, or audit-log change.Why
Fixes #196. Claim's four graph-reference fields were validated by no write path:
put_claim(src/vouch/storage.py:348) checked onlyclaim.evidence.update_claim(src/vouch/storage.py:383) re-validated the model (the fix(models): require Claim.evidence to be non-empty at the model layer #82 fix) but the model validator has no KB access, so dangling refs sailed through.bundle.import_applywrites claim YAML straight to disk (dest.write_bytes, noput_claim);import_checkvalidated onlyclaim.evidencefor claims.proposals.approve→put_claim, inheriting the gap.The #124 graph-integrity fix closed
Relation.source/target/evidenceandPage.entities/sources— and its own comment (src/vouch/storage.py:314) lists exactly those while silently omitting the Claim's own fields. Meanwhilefsck._check_lifecycle_chains(src/vouch/health.py:251-288) already declaresdangling_supersedes/dangling_superseded_by/dangling_contradictsas error-severity findings. So the contract was articulated but enforced by no writer — the identical shape as #81 (Claim.evidencenon-empty) and #123/#124 (Relation/Page dangling refs).Claim.entitieshad zero coverage anywhere — not at write time, not infsck.What might break
Strictly additive write-time validation. Honest writes are unaffected; only Claims whose graph refs point at non-existent artifacts start being rejected.
store.put_claim/store.update_claimraiseValueError("claim <id> references unknown entity/claim …")for a danglingentities/supersedes/superseded_by/contradicts.bundle.import_checkreports adangling reference: … claim entity …/… claim graph ref …issue andimport_applyrefuses (it raises on the firstimport_checkissue).supersede/contradict(src/vouch/lifecycle.py:35-36,74-75) load both ends viaget_claimbefore linking, so their refs always resolve.vouch fsckexactly as before. The migration story matches bug: Claim model has no min-evidence validator — uncited claims land via bundle import, put_claim, update_claim #81 / validation gap: every write path lands Relations/Pages with dangling foreign-id references #123.VEP
Not required — tightens write-time validation to match an invariant
fsckalready documents; no surface change.Tests
tests/test_storage.py,tests/test_health.py,tests/test_bundle.py,tests/test_cli.py(the touched test).ruff check src testsclean;mypy src→Success: no issues found. (The single failingtest_bundle.py::test_import_apply_rejects_absolute_pathis pre-existing ontest— a Windows-specific path-message assertion unrelated to this change; verified it fails identically with these changes stashed.)tests/test_storage.py(7):test_put_claim_rejects_unknown_{entity,supersedes,superseded_by,contradicts}_ref,test_put_claim_accepts_resolvable_graph_refs,test_update_claim_rejects_in_place_mutation_to_dangling_ref,test_lifecycle_contradict_round_trips_after_guard.tests/test_bundle.py(2):test_import_check_rejects_claim_with_dangling_graph_refs,test_import_check_accepts_claim_with_resolvable_graph_refs(honest round-trip guard).tests/test_health.py/tests/test_cli.py: the existingfsckdangling-chain tests now write poisoned claim YAML directly to disk (via a_write_claim_directhelper) so they reproduce the legacy on-disk statefsckmust still surface, rather than relying onput_claimto plant data it now correctly rejects; the asymmetric-contradicts test creates its claims in dependency order.CHANGELOG.mdupdated under## [Unreleased] ### Fixed.Closes #196.
🤖 Generated with Claude Code