fix(models): require Claim.evidence to be non-empty at the model layer#82
Conversation
vouchdev#81) The 'claims must cite sources' guarantee (README §'Why this exists' point 3; CONTRIBUTING §'Things we won't merge') used to live only in proposals.propose_claim, so every other write path silently accepted Claim(evidence=[]) and landed an uncited claim: - store.put_claim direct: existence-check loop iterates zero times. - store.update_claim: writes the YAML without re-validating. - bundle.import_apply via _validate_content: defers to Claim.model_validate, which accepted evidence=[] because the model had no min-length constraint. Add @field_validator('evidence') on Claim — raises ValueError when the list is empty. Closes all three bypass paths in one place. store.update_claim additionally re-validates via Claim.model_validate(claim.model_dump()) before persisting, so in-place mutation (c.evidence = []; store.update_claim(c)) raises before the YAML hits disk — the field validator only fires at construction time, not on attribute assignment. Four regression tests: - test_claim_model_rejects_empty_evidence (tests/test_storage.py) — Claim(evidence=[]) raises pydantic.ValidationError. - test_put_claim_rejects_empty_evidence — store.put_claim raises; no claims/<id>.yaml is written. - test_update_claim_rejects_empty_evidence — in-place mutation + update_claim raises; the on-disk YAML is unchanged. - test_import_rejects_uncited_claim (tests/test_bundle.py) — a schema-valid bundle whose claim YAML has evidence: [] is rejected by import_check (schema validation issue) and import_apply raises before writing. The existing guard in proposals.propose_claim becomes a redundant user-facing error message and is left in place for the friendlier CLI/JSONL error string. No on-disk-layout, schema, or bundle-format change; data the model never should have accepted now raises.
|
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:
📝 WalkthroughWalkthroughMove the "claims must have citations" invariant into the Claim Pydantic model, re-run validation on update, make lint resilient to legacy uncited claim YAMLs, and add tests plus a changelog entry documenting behavior and migration impact. ChangesClaim evidence validation enforcement
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 2
🧹 Nitpick comments (1)
src/vouch/storage.py (1)
321-327: ⚡ Quick winUse the validated Claim instead of discarding it.
The re-validation on line 326 correctly catches invalid mutations, but the validated result is discarded. Line 327 still dumps the original
claimobject. For the current_at_least_one_citationvalidator this works fine (it only validates, doesn't transform), but if future validators transform data, those transformations would be lost.♻️ Refactor to use the validated result
- # Re-validate the in-memory Claim before persisting so model - # invariants (e.g. evidence must be non-empty — see `#81`) hold - # even when a caller mutated fields in place after get_claim(). - # The Claim model's field validators only run at construction - # time; mutation alone bypasses them unless we round-trip. - Claim.model_validate(claim.model_dump(mode="json")) - self._claim_path(claim.id).write_text(_yaml_dump(claim.model_dump(mode="json"))) + # Re-validate the in-memory Claim before persisting so model + # invariants (e.g. evidence must be non-empty — see `#81`) hold + # even when a caller mutated fields in place after get_claim(). + # The Claim model's field validators only run at construction + # time; mutation alone bypasses them unless we round-trip. + validated = Claim.model_validate(claim.model_dump(mode="json")) + self._claim_path(claim.id).write_text(_yaml_dump(validated.model_dump(mode="json")))This also eliminates the double serialization of the claim object.
🤖 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 321 - 327, The code currently calls Claim.model_validate(claim.model_dump(...)) but discards the validated return value and still writes the original claim; change this to capture the validated model (e.g., validated = Claim.model_validate(claim.model_dump(mode="json"))) and then persist the validated model by calling self._claim_path(claim.id).write_text(_yaml_dump(validated.model_dump(mode="json"))), so any validator transformations are preserved and you avoid double-serializing the original object.
🤖 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_bundle.py`:
- Around line 338-343: Tighten the assertions to verify the citation-specific
error about empty evidence rather than any schema failure: update the
diff.issues check to assert that at least one issue string contains both "schema
validation failed" and the citation invariant text (e.g., "cite at least one"),
and update the pytest.raises match to include both substrings (or a regex
matching "schema validation failed" and "cite at least one") when calling
bundle.import_apply; reference the existing call sites bundle.import_check,
diff.issues, and bundle.import_apply to locate the assertions to change.
In `@tests/test_storage.py`:
- Around line 122-129: The test currently constructs Claim(id="c1",
text="uncited", evidence=[]) which triggers model validation before
store.put_claim is called; change the test to construct a valid Claim (e.g.,
evidence with at least one item), then mutate that Claim's evidence to an empty
list and call store.put_claim(…) expecting ValidationError; keep the final
assertion that (store.kb_dir / "claims" / "c1.yaml").exists() is false and
reference the test function name test_put_claim_rejects_empty_evidence and the
store.put_claim and Claim symbols when making the change.
---
Nitpick comments:
In `@src/vouch/storage.py`:
- Around line 321-327: The code currently calls
Claim.model_validate(claim.model_dump(...)) but discards the validated return
value and still writes the original claim; change this to capture the validated
model (e.g., validated = Claim.model_validate(claim.model_dump(mode="json")))
and then persist the validated model by calling
self._claim_path(claim.id).write_text(_yaml_dump(validated.model_dump(mode="json"))),
so any validator transformations are preserved and you avoid double-serializing
the original object.
🪄 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: d8ff8dfd-d140-40b8-9788-d75291902b43
📒 Files selected for processing (5)
CHANGELOG.mdsrc/vouch/models.pysrc/vouch/storage.pytests/test_bundle.pytests/test_storage.py
| diff = bundle.import_check(store.kb_dir, bundle_path) | ||
| assert not diff.ok | ||
| assert any("schema validation failed" in i for i in diff.issues), diff.issues | ||
|
|
||
| with pytest.raises(RuntimeError, match="schema validation failed"): | ||
| bundle.import_apply(store.kb_dir, bundle_path) |
There was a problem hiding this comment.
Assert the citation-specific error, not only a generic schema failure.
The current checks can pass on unrelated schema errors. To keep this regression targeted to empty evidence, assert that the message also includes the citation invariant text (e.g., "cite at least one").
Suggested assertion tightening
diff = bundle.import_check(store.kb_dir, bundle_path)
assert not diff.ok
- assert any("schema validation failed" in i for i in diff.issues), diff.issues
+ assert any(
+ "schema validation failed" in i and "cite at least one" in i
+ for i in diff.issues
+ ), diff.issues
- with pytest.raises(RuntimeError, match="schema validation failed"):
+ with pytest.raises(RuntimeError, match=r"schema validation failed.*cite at least one"):
bundle.import_apply(store.kb_dir, bundle_path)🤖 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 338 - 343, Tighten the assertions to
verify the citation-specific error about empty evidence rather than any schema
failure: update the diff.issues check to assert that at least one issue string
contains both "schema validation failed" and the citation invariant text (e.g.,
"cite at least one"), and update the pytest.raises match to include both
substrings (or a regex matching "schema validation failed" and "cite at least
one") when calling bundle.import_apply; reference the existing call sites
bundle.import_check, diff.issues, and bundle.import_apply to locate the
assertions to change.
| def test_put_claim_rejects_empty_evidence(store: KBStore) -> None: | ||
| """Regression for #81: store.put_claim is a direct write path | ||
| that used to silently accept Claim(evidence=[]) because the | ||
| only existence-check loop iterated zero times. The model-level | ||
| validator now fires before put_claim is even called.""" | ||
| with pytest.raises(ValidationError, match="cite at least one"): | ||
| store.put_claim(Claim(id="c1", text="uncited", evidence=[])) | ||
| assert not (store.kb_dir / "claims" / "c1.yaml").exists() |
There was a problem hiding this comment.
This test can pass without ever exercising store.put_claim validation.
On Line 128, Claim(id="c1", text="uncited", evidence=[]) raises before store.put_claim(...) is called, so this does not verify the write-path behavior. Use a valid claim, mutate evidence to [], then call store.put_claim to catch mutation-based bypasses.
Proposed test adjustment
def test_put_claim_rejects_empty_evidence(store: KBStore) -> None:
@@
- with pytest.raises(ValidationError, match="cite at least one"):
- store.put_claim(Claim(id="c1", text="uncited", evidence=[]))
+ src = store.put_source(b"e")
+ claim = Claim(id="c1", text="uncited", evidence=[src.id])
+ claim.evidence = []
+ with pytest.raises(ValidationError, match="cite at least one"):
+ store.put_claim(claim)
assert not (store.kb_dir / "claims" / "c1.yaml").exists()📝 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.
| def test_put_claim_rejects_empty_evidence(store: KBStore) -> None: | |
| """Regression for #81: store.put_claim is a direct write path | |
| that used to silently accept Claim(evidence=[]) because the | |
| only existence-check loop iterated zero times. The model-level | |
| validator now fires before put_claim is even called.""" | |
| with pytest.raises(ValidationError, match="cite at least one"): | |
| store.put_claim(Claim(id="c1", text="uncited", evidence=[])) | |
| assert not (store.kb_dir / "claims" / "c1.yaml").exists() | |
| def test_put_claim_rejects_empty_evidence(store: KBStore) -> None: | |
| """Regression for `#81`: store.put_claim is a direct write path | |
| that used to silently accept Claim(evidence=[]) because the | |
| only existence-check loop iterated zero times. The model-level | |
| validator now fires before put_claim is even called.""" | |
| src = store.put_source(b"e") | |
| claim = Claim(id="c1", text="uncited", evidence=[src.id]) | |
| claim.evidence = [] | |
| with pytest.raises(ValidationError, match="cite at least one"): | |
| store.put_claim(claim) | |
| assert not (store.kb_dir / "claims" / "c1.yaml").exists() |
🤖 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 122 - 129, The test currently constructs
Claim(id="c1", text="uncited", evidence=[]) which triggers model validation
before store.put_claim is called; change the test to construct a valid Claim
(e.g., evidence with at least one item), then mutate that Claim's evidence to an
empty list and call store.put_claim(…) expecting ValidationError; keep the final
assertion that (store.kb_dir / "claims" / "c1.yaml").exists() is false and
reference the test function name test_put_claim_rejects_empty_evidence and the
store.put_claim and Claim symbols when making the change.
|
My question before merging: since the validator now also fires when claims are read back, a KB that already has an uncited claim from the old bug would start throwing on |
…hdev#82 review) The new Claim.evidence min-citation validator (vouchdev#81) also fires when claims are read back from disk. A KB that has a pre-existing uncited claims/<id>.yaml from before the fix would otherwise crash vouch lint / vouch doctor with a bare pydantic.ValidationError deep in store.list_claims(). Add _load_claims_for_lint(), a per-file iteration that catches pydantic.ValidationError (and any other load error) and surfaces each bad file as a Finding with code='invalid_claim' and an explicit repair hint: 'edit the YAML to add a citation, or delete the file'. lint() also stops calling status() to populate counts — status() calls the strict store.list_claims() which would re-raise on the same files — and builds the counts dict inline from the safely-loaded valid claims. Regression test in tests/test_health.py: - test_lint_surfaces_legacy_uncited_claim_yaml_without_crashing hand-crafts a claims/legacy.yaml with evidence: [] (matches the on-disk shape an older buggy write path would have left), asserts vouch lint runs to completion, surfaces invalid_claim in findings with the repair-hint message, and that the well-formed sibling claim is still discovered. CHANGELOG migration note expanded to describe the repair hint.
|
Good catch — agreed that a bare What changed in 5c881de:
The strict validator on the write paths is unchanged — every new Regression test ( CHANGELOG migration note updated to describe the repair hint. Full suite: ruff clean, 5/5 |
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 `@src/vouch/health.py`:
- Around line 143-160: The counts dict mixes non-int types causing type errors:
remove or move "kb_dir" out of HealthReport.counts and ensure every value in
counts is an int (e.g., replace "index_present": (store.kb_dir /
index_db.DB_FILENAME).exists() with "index_present": int((store.kb_dir /
index_db.DB_FILENAME).exists()) and convert any other non-int entries
similarly); update construction near the end of the function that builds counts
and return HealthReport(ok=ok, findings=findings, counts=counts) (or add a
separate field on HealthReport for kb_dir if you need to keep it).
🪄 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: 415ab105-dd99-42ba-b9fb-86063ead19fa
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/health.pytests/test_health.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
The new inline-built counts in lint() (from 5c881de) is a literal dict with mixed value types (str/int/bool), which mypy correctly inferred as dict[str, object] and rejected against the HealthReport.counts: dict[str, int] annotation. The original status() returned the same mixed dict via an untyped 'dict' return, which masked the mismatch — the narrow type was effectively never checked at the call site. Widen counts to dict[str, Any] to match runtime reality. Also tighten status()'s return annotation from 'dict' to 'dict[str, Any]' for consistency. No caller does arithmetic on counts values; they just echo or pass through, so the widening is risk-free.
|
Create PR against the test branch |
|
@plind-junior — retargeted to |
|
Fix the conflict, other than that LGTM! |
Conflict resolved. |
…hdev#82 review) The new Claim.evidence min-citation validator (vouchdev#81) also fires when claims are read back from disk. A KB that has a pre-existing uncited claims/<id>.yaml from before the fix would otherwise crash vouch lint / vouch doctor with a bare pydantic.ValidationError deep in store.list_claims(). Add _load_claims_for_lint(), a per-file iteration that catches pydantic.ValidationError (and any other load error) and surfaces each bad file as a Finding with code='invalid_claim' and an explicit repair hint: 'edit the YAML to add a citation, or delete the file'. lint() also stops calling status() to populate counts — status() calls the strict store.list_claims() which would re-raise on the same files — and builds the counts dict inline from the safely-loaded valid claims. Regression test in tests/test_health.py: - test_lint_surfaces_legacy_uncited_claim_yaml_without_crashing hand-crafts a claims/legacy.yaml with evidence: [] (matches the on-disk shape an older buggy write path would have left), asserts vouch lint runs to completion, surfaces invalid_claim in findings with the repair-hint message, and that the well-formed sibling claim is still discovered. CHANGELOG migration note expanded to describe the repair hint.
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.
2 participants
Problem
The
Claimmodel has no min-evidence validator(
src/vouch/models.py:184-187), so aClaimwithevidence: []isschema-valid. The README and CONTRIBUTING contract —
is enforced in exactly one place,
proposals.propose_claim(
src/vouch/proposals.py:89-90). Every other write path silentlyaccepts an uncited claim:
store.put_claimdirect (src/vouch/storage.py:283-291) —the existence-check loop iterates zero times.
store.update_claim— writes the YAML without re-validating.bundle.import_apply—_validate_contentdefers toClaim.model_validate, which acceptsevidence: []; themalicious bundle passes
import_check(ok=True) and lands theuncited claim under the legitimate
bundle_id, with a cleanbundle.importaudit event.CONTRIBUTING explicitly lists this as a won't merge category:
— except the enforcement was never on the model in the first
place, so every write path except the one explicit gate already
qualifies as the relaxed shape the team has stated they reject.
End-to-end repro on
main(uncited_repro.py):Fix
src/vouch/models.py, on theClaimmodel:That closes the direct-construction path, the bundle import path
(both go through
Claim(...)/Claim.model_validate(...)), andthe
propose_claimpath (the existing string-comparison guardbecomes a redundant nicer-error message and is left in place).
The in-place mutation path (
c.evidence = []; store.update_claim(c))needs one extra line:
store.update_claimnow doesClaim.model_validate(claim.model_dump(mode="json"))beforepersisting. Pydantic field validators only fire at construction
time, so mutation alone bypasses them; the round-trip catches it
before the YAML hits disk.
The existing user-facing guard in
proposals.propose_claimisleft as a friendlier CLI/JSONL error message. No on-disk-layout,
schema, or bundle-format change — data the model never should
have accepted now raises.
Tests
Four regression tests:
test_claim_model_rejects_empty_evidence(tests/test_storage.py)—
Claim(evidence=[])raisespydantic.ValidationErrorwithmessage containing
"cite at least one".test_put_claim_rejects_empty_evidence—store.put_claimraises;no
claims/<id>.yamlis written.test_update_claim_rejects_empty_evidence— in-place mutationfollowed by
store.update_claimraises; the on-disk YAML isbyte-for-byte unchanged.
test_import_rejects_uncited_claim(tests/test_bundle.py) —a manifest-consistent bundle whose claim YAML has
evidence: []is rejected by
import_check(with aschema validation failed:issue), and
import_applyraisesRuntimeError("refusing to import: schema validation failed: ...")before writing.All 4 pass on Windows (Python 3.12.13). Lint clean
(
ruff check src tests). Full suite shows the same pre-existingWindows-only failures present on
main(O_NOFOLLOWinstorage.py, bundle round-trip CRLF) — both untouched by this PR.Test count: 152 → 156 (+4 new, 0 regressions).
Audited every existing
Claim(...)call site intests/beforeadding the validator — none construct claims with empty evidence,
so the new gate breaks no existing test.
Fixes #81
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Health