fix(storage): reject dangling Claim graph references on every write path - #201
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.
c82a5f7 to
d3a134b
Compare
|
@codex review |
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 👍 / 👎.
|
@galuis116 Would you fix codex review? |
…check, fsck coverage - lifecycle.supersede / contradict pre-validate both touched claims via _validate_claim_refs before the first disk write so a legacy dangling ref can't half-apply the operation (P2 vouchdev#3). - proposals.check_approvable dry-runs the put_*-side ref guards via a new _payload_block_reason helper so the default `vouch approve a b` batch flow catches a dangling claim.entities ref BEFORE any write, preserving the all-or-nothing contract (P2 vouchdev#1). - health.fsck reports claim.entities pointing at a missing entity as a new `dangling_claim_entity` error finding, giving operators a preflight repair path for legacy KBs (P2 vouchdev#2).
|
@plind-junior Pushed P2 #3 — P2 #1 — P2 #2 — fsck coverage for Diff: +343 / −3 across Tests (6 new in
CHANGELOG entry added under |
|
@galuis116 Thanks for the contribution. Would you optimize the comments and description in this PR? |
|
@plind-junior Done — pushed |
What changed
KBStore._validate_claim_refsrejects a Claim whoseentities,supersedes,superseded_by, orcontradictspoint at an artifact not in the KB, called from bothput_claimandupdate_claim.bundle.import_checkgains the matching check on the claim branch.lifecycle.supersede/contradictpre-validate both touched claims before the first disk write (atomicity).proposals.check_approvabledry-runs the same ref guards sovouch approve a bstays all-or-nothing.vouch fsckreportsclaim.entitiespointing at a missing entity as a newdangling_claim_entityerror finding.Why
Fixes #196.
Claim's four graph-ref fields were validated by no writer —put_claimchecked onlyevidence,update_claim's model re-check has no KB access,bundle.import_applywrites claim YAML directly, andproposals.approveinherits the gap. Meanwhilefsckalready declaresdangling_supersedes/_superseded_by/_contradictsas error-severity findings, so the invariant was articulated but enforced by no writer — same shape as #81 / #123 / #124.What might break
Strictly additive write-time validation. Honest writes unaffected; only Claims with dangling graph refs start being rejected.
put_claim/update_claimraiseValueError("claim <id> references unknown entity/claim …").bundle.import_checkreports adangling reference: … claim …issue;import_applyrefuses.lifecycle.supersede/contradictraise before any disk write if either claim has a dangling ref (atomicity).check_approvablereturns the same reason for batch precheck.vouch fsck(now includingdangling_claim_entity).VEP
Not required — tightens write-time validation to match an invariant
fsckalready documents.Tests
make checkpasses locally (ruff, mypy 54 files, affected pytest files green).tests/test_storage.py(graph-ref guard, lifecycle atomicity,check_approvableprecheck),tests/test_bundle.py(bundle reject + accept),tests/test_health.py(dangling_claim_entity+ existing dangling-chain tests now write claim YAML directly).CHANGELOG.mdupdated under## [Unreleased] ### Fixed.Closes #196.