fix(validation): reject dangling foreign-id refs on every write path - #152
fix(validation): reject dangling foreign-id refs on every write path#152greatjourney589 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds validation across three layers of the Vouch system to prevent Relations and Pages from referencing non-existent artifacts. The storage layer validates endpoints and evidence; the proposals layer performs early validation; bundle imports run cross-artifact graph integrity checks; and comprehensive tests verify all write paths reject dangling references. ChangesReferential Integrity Validation Across Write Paths
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProposalAPI as propose_relation<br/>propose_page
participant StorageAPI as put_relation<br/>put_page
participant BundleAPI as import_check
participant KBStore as KB Store
Client->>ProposalAPI: submit relation/page with artifact IDs
ProposalAPI->>KBStore: _resolve_artifact(id)
KBStore-->>ProposalAPI: exists? true/false
ProposalAPI->>StorageAPI: put_relation/put_page
StorageAPI->>KBStore: _relation_referable(id)
KBStore-->>StorageAPI: resolvable? true/false
StorageAPI->>KBStore: write YAML to disk
Client->>BundleAPI: import bundle with relations/pages
BundleAPI->>KBStore: _check_graph_integrity(bundle_members)
KBStore-->>BundleAPI: dangling refs found? error list
BundleAPI-->>Client: import check result (ok/issues)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/vouch/storage.py (1)
436-443: ⚡ Quick win
_validate_relationruns before the idempotent existence short-circuit.The docstring promises this path can "converge to a consistent state on retry without raising if the relation file was already written." But
_validate_relationis now called unconditionally before thepath.exists()early-return. If an already-written relation is re-applied after one of its endpoints was removed, the converge/retry now raisesValueErrorinstead of succeeding — weakening the idempotency guarantee. Consider validating only when actually writing a new file.♻️ Move validation after the existence check
- self._validate_relation(rel) path = self._relation_path(rel.id) if path.exists(): self._embed_and_store( kind="relation", id=rel.id, text=f"{rel.source} {rel.relation.value} {rel.target}", ) return rel + self._validate_relation(rel) try: with path.open("x") as f:Please confirm whether lifecycle ops (supersede/contradict) can re-invoke
put_relation_idempotentfor an existing relation after an endpoint deletion, which would make the eager validation a regression.🤖 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/storage.py` around lines 436 - 443, The code calls self._validate_relation(rel) before checking if the relation file already exists via path = self._relation_path(rel.id) and path.exists(), which breaks idempotency; change the flow in put_relation_idempotent (or the method containing this diff) to perform the existence short-circuit first (compute path and if path.exists(): return rel) and only call self._validate_relation(rel) and self._embed_and_store(...) when the file does not exist, so validation only runs on writes and preserves the promised converge-on-retry behavior.tests/test_bundle.py (1)
407-432: ⚡ Quick winMove repeated import to module level.
The import
from vouch.storage import _serialize_pageappears twice (lines 411 and 425). Move it to the top of the file alongside other imports for cleaner code organization.♻️ Proposed fix
At the top of the file (after line 16), add:
from vouch.models import Claim, Entity, EntityType, Page, Relation, RelationType from vouch.storage import KBStore +from vouch.storage import _serialize_pageThen remove the inline imports at lines 411 and 425:
def test_import_check_flags_page_with_unknown_entity( store: KBStore, tmp_path: Path ) -> None: page = Page(id="p1", title="T", entities=["ghost-entity"]) - from vouch.storage import _serialize_page page_bytes = _serialize_page(page).encode()def test_import_check_flags_page_with_unknown_source( store: KBStore, tmp_path: Path ) -> None: page = Page(id="p1", title="T", sources=["deadbeef" * 8]) - from vouch.storage import _serialize_page page_bytes = _serialize_page(page).encode()🤖 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_bundle.py` around lines 407 - 432, The tests currently import _serialize_page inline in two places inside test_import_check_flags_page_with_unknown_entity and test_import_check_flags_page_with_unknown_source; move the import "from vouch.storage import _serialize_page" to the module-level imports at the top of tests/test_bundle.py (alongside the other imports) and remove the two duplicate inline imports inside those test functions so both tests use the single top-level _serialize_page import.
🤖 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/bundle.py`:
- Around line 302-319: The loop that validates pages in bundle import only
checks page.entities and page.sources but not page.claims, so imported pages can
reference missing claims; in the loop that deserializes pages (using
_deserialize_page over bundle_members) add a check iterating page.claims and for
each cid not in claim_ids append an issue like the existing messages (e.g.,
"graph integrity: page {page.id} references unknown claim {cid!r}") so
page→claim references are validated against the merged claim_ids set just like
entities and sources.
---
Nitpick comments:
In `@src/vouch/storage.py`:
- Around line 436-443: The code calls self._validate_relation(rel) before
checking if the relation file already exists via path =
self._relation_path(rel.id) and path.exists(), which breaks idempotency; change
the flow in put_relation_idempotent (or the method containing this diff) to
perform the existence short-circuit first (compute path and if path.exists():
return rel) and only call self._validate_relation(rel) and
self._embed_and_store(...) when the file does not exist, so validation only runs
on writes and preserves the promised converge-on-retry behavior.
In `@tests/test_bundle.py`:
- Around line 407-432: The tests currently import _serialize_page inline in two
places inside test_import_check_flags_page_with_unknown_entity and
test_import_check_flags_page_with_unknown_source; move the import "from
vouch.storage import _serialize_page" to the module-level imports at the top of
tests/test_bundle.py (alongside the other imports) and remove the two duplicate
inline imports inside those test functions so both tests use the single
top-level _serialize_page import.
🪄 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: 405f0a42-3a0f-4a35-a1e1-8768c32d98a7
📒 Files selected for processing (6)
src/vouch/bundle.pysrc/vouch/proposals.pysrc/vouch/storage.pytests/test_bundle.pytests/test_health.pytests/test_storage.py
ReviewSummary: Adds referential-integrity enforcement across all write paths for What works
Suggestions
VerdictRequest changes — The |
|
Hi,@plind-junior |
What changed
Added foreign-ID reference validation on every write path that touches relations, pages, and proposals.
storage.put_relationandstorage.put_pagenow verify that all referenced entity, source, claim, and evidence IDs actually exist before writing.proposals.propose_relationandpropose_pagedo the same at proposal time.bundle.import_checkgained a new_check_graph_integritypass that mirrors these invariants for incoming bundle members against the merged (disk + bundle) ID space.Why
Previously, it was possible to write a relation pointing to a non-existent entity or a page referencing an unknown source without any error. This left the KB in a state where graph traversal and export would silently produce incomplete or incorrect results.
Fixes #123.
What might break
No on-disk layout or field shape changes. Existing
.vouch/directories are unaffected on read. However, any code that previously wrote relations or pages with dangling foreign IDs will now receive aValueError(storage layer) orProposalError(proposals layer). Bundle imports that contain dangling refs will now produceissuesentries and returnok=Falsefromimport_check.VEP
Not required — no surface change (no new
kb.*methods, no model field additions, no on-disk format change).Tests
make checkpasses locally (lint + mypy + pytest)CHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
Release Notes
New Features
Tests