Skip to content

fix(models): reject empty text/name/title on Claim/Entity/Page#300

Open
minion1227 wants to merge 2 commits into
vouchdev:mainfrom
minion1227:minion_155
Open

fix(models): reject empty text/name/title on Claim/Entity/Page#300
minion1227 wants to merge 2 commits into
vouchdev:mainfrom
minion1227:minion_155

Conversation

@minion1227

@minion1227 minion1227 commented Jul 1, 2026

Copy link
Copy Markdown

What changed

Claim.text, Entity.name, and Page.title now reject empty /
whitespace-only values at the model layer, via @field_validators that
mirror the existing Claim.evidence min-citation validator (#81/#82).

Why

the non-empty contract for these fields lived only in the propose_*
helpers, so three other write paths slipped through:

  • direct constructionClaim(text=""), Entity(name=""),
    Page(title="") were all accepted, and store.put_* landed the
    YAML / markdown on disk.
  • bundle / sync importbundle._validate_content calls
    <Model>.model_validate(...), which had no text-content validator, so a
    manifest-consistent bundle with blank fields imported cleanly.
  • mutate-then-updateclaim.text = ""; store.update_claim(claim)
    re-validates via model_validate, but with no validator the blank
    persisted.

enforcing on the model closes all three at once — exactly the
contract-vs-enforcement asymmetry that #81/#82 fixed for Claim.evidence.
closes #155.

What might break

nothing on disk or on the wire. this only rejects artifacts that were never
semantically valid (blank text / name / title). existing well-formed KBs are
unaffected. the validators gate on emptiness only and preserve the
surrounding whitespace of non-blank values (no silent stripping). a KB that
somehow already holds a blank-field artifact will surface it as a
validation error on next load — which is the intended signal, matching how
the #81 evidence validator behaves.

Source.locator is intentionally out of scope (the issue flags its
format question as bigger and deserving its own issue).

VEP

not a surface change — a model-layer validation tightening in the same shape
already merged for Claim.evidence. no VEP needed.

Tests

  • make check — ruff clean; mypy clean on touched files; pytest green
    (the only local failures are pre-existing and environment-only:
    fastapi/[web] not installed and embeddings installed, both of
    which also fail on main)
  • new behaviour has tests in tests/test_storage.py (alongside the
    bug: Claim model has no min-evidence validator — uncited claims land via bundle import, put_claim, update_claim #81 evidence-validator tests): model-construction rejection
    (empty + whitespace, parametrised), put_* rejection with
    no-file-landed assertions, update_claim mutate-then-persist
    rejection, and a whitespace-preservation guard for non-blank values
  • CHANGELOG.md updated under ## [Unreleased]

Summary by CodeRabbit

  • Bug Fixes
    • Empty or whitespace-only values are now rejected for claim text, entity names, and page titles.
    • Storage operations now consistently block blank values from being created or updated, closing gaps in non-proposal write paths.
    • Added regression tests ensuring invalid inputs are rejected while valid text remains unchanged.

the non-empty contract for `Claim.text`, `Entity.name`, and `Page.title`
lived only in the `propose_*` helpers, so `store.put_*`, `store.update_*`,
and bundle/sync import (via `_validate_content`) all silently accepted
empty or whitespace-only values and landed artifacts carrying zero of the
field's semantic content.

add `@field_validator`s on each field, mirroring the existing
`Claim.evidence` min-citation validator (vouchdev#81/vouchdev#82): they reject blank input
at construction time, so every write path — direct construction,
put/update, and import — inherits the check at once. the validators gate
on emptiness only and preserve surrounding whitespace of non-blank values.

`Source.locator` is deliberately left out of scope — its format question
is bigger and deserves its own issue, as the report notes.

closes vouchdev#155
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 1, 2026
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c88f6183-d3af-44ba-b2a8-7167d8e9ef82

📥 Commits

Reviewing files that changed from the base of the PR and between 536c4ce and 89815c0.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • src/vouch/models.py
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md

📝 Walkthrough

Walkthrough

Adds model-level validators that reject empty or whitespace-only Claim.text, Entity.name, and Page.title. Storage regression tests cover construction and write/update paths, and the changelog records the validation change.

Changes

Model-layer non-empty validation

Layer / File(s) Summary
Claim.text validator and coverage
src/vouch/models.py, tests/test_storage.py
Adds a shared non-empty helper and a Claim.text validator; tests cover blank-value rejection, put_claim/update_claim behavior, and whitespace preservation.
Entity.name validator and coverage
src/vouch/models.py, tests/test_storage.py
Adds an Entity.name validator; tests cover blank-value rejection and put_entity behavior.
Page.title validator, coverage, and changelog
src/vouch/models.py, tests/test_storage.py, CHANGELOG.md
Adds a Page.title validator; tests cover blank-value rejection and put_page behavior, and the changelog notes the fix.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related issues

Poem

A bunny hops through fields of code,
To guard each name and title load.
No blank little whispers slip through the wire,
Just honest words and validators that don’t tire. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the core change: rejecting empty text/name/title in Claim, Entity, and Page models.
Linked Issues check ✅ Passed The validators and tests address the linked gap by enforcing non-empty values at the model layer for all three fields.
Out of Scope Changes check ✅ Passed The changelog update, helper extraction, and regression tests all support the stated validation fix.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/vouch/models.py (1)

235-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting shared non-empty validator helper.

_text_non_empty, _name_non_empty, and _title_non_empty are structurally identical (strip-check + raise ValueError). A small shared helper (e.g. _require_non_empty(v: str, field_label: str) -> str) called from each @field_validator would remove the duplication while keeping the per-field error messages.

♻️ Proposed refactor
+def _require_non_empty(v: str, label: str) -> str:
+    if not v.strip():
+        raise ValueError(f"{label} must not be empty")
+    return v
+
+
 class Claim(BaseModel):
     ...
     `@field_validator`("text")
     `@classmethod`
     def _text_non_empty(cls, v: str) -> str:
-        if not v.strip():
-            raise ValueError("claim text must not be empty")
-        return v
+        return _require_non_empty(v, "claim text")

Apply the same pattern to Entity._name_non_empty and Page._title_non_empty.

Also applies to: 281-288, 338-345

🤖 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/models.py` around lines 235 - 245, Extract the duplicated
strip-and-empty validation logic in the model validators into a shared helper
such as _require_non_empty(v, field_label), then have _text_non_empty,
Entity._name_non_empty, and Page._title_non_empty call it so each field keeps
its own error message without repeating the same check. Keep the existing field
validators in place and replace the repeated if not v.strip(): raise
ValueError(...) blocks with the shared helper to reduce duplication across the
affected validators.
🤖 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 `@CHANGELOG.md`:
- Line 99: Capitalize the sentence start in the changelog entry so the text
after the period begins with “The” instead of “the”. Update the affected
CHANGELOG.md entry only, keeping the rest of the release note unchanged.

---

Nitpick comments:
In `@src/vouch/models.py`:
- Around line 235-245: Extract the duplicated strip-and-empty validation logic
in the model validators into a shared helper such as _require_non_empty(v,
field_label), then have _text_non_empty, Entity._name_non_empty, and
Page._title_non_empty call it so each field keeps its own error message without
repeating the same check. Keep the existing field validators in place and
replace the repeated if not v.strip(): raise ValueError(...) blocks with the
shared helper to reduce duplication across the affected validators.
🪄 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: dac51ccc-7071-47be-a22f-7902ad39d843

📥 Commits

Reviewing files that changed from the base of the PR and between 94e84b1 and 536c4ce.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • src/vouch/models.py
  • tests/test_storage.py

Comment thread CHANGELOG.md Outdated
address review on vouchdev#300: the three vouchdev#155 field validators
(Claim.text / Entity.name / Page.title) were structurally identical
strip-checks. extract a module-level `_require_non_empty(v, label)` helper
they all delegate to, keeping the per-field error messages. also fix a
mid-entry sentence casing in the CHANGELOG. no behaviour change.
@minion1227

Copy link
Copy Markdown
Author

thanks — addressed both in 89815c0:

  • extracted a shared module-level _require_non_empty(v, label) helper; the
    three validators now delegate to it, keeping their per-field messages.
  • fixed the mid-entry sentence casing in the CHANGELOG.

no behaviour change; tests and assertions unchanged and green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs documentation, specs, examples, and repo guidance size: S 50-199 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validation gap: Claim.text, Page.title, Entity.name accept empty/whitespace at the model layer; only propose_* enforces non-empty

1 participant