fix(proposals): validate claim/relation/entity payloads at propose time#509
fix(proposals): validate claim/relation/entity payloads at propose time#509tryeverything24 wants to merge 1 commit into
Conversation
propose_claim, propose_relation, and propose_entity built their payload dict and handed it straight to _file_proposal with no check against the Claim/Relation/Entity model's own constraints (confidence Field(ge=0.0, le=1.0), the RelationType/EntityType enums). approve() was the only place that ever caught it, via Claim(**payload) etc — too late to give the proposer a usable error, and too late to keep the pending queue clean, since a proposal that fails that check can never be approved and just sits there. validates with the same Model(**payload) construction _payload_block_reason and approve() already use, so any model-level constraint is caught, not just confidence. mirrors the propose-time validation propose_page and propose_relation's existing endpoint/evidence checks already do for other fields — same pattern, closing the one field class that slipped through it. validation commands run: pytest tests/ -q --ignore=tests/embeddings (1431 passed), mypy src (clean), ruff check src tests (clean).
WalkthroughProposal functions now validate claim, entity, and relation payloads against their models before filing proposals. Invalid confidence values and entity types raise ChangesProposal validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/vouch/proposals.py (1)
149-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate validation logic into a helper function.
The payload validation and error conversion block is repeated identically across three proposal functions. Consider extracting this into a shared helper to keep the functions focused and ensure error formatting remains consistent.
src/vouch/proposals.py#L149-L152: replace with a call to the helper, e.g.,_validate_payload(Claim, payload, "claim").src/vouch/proposals.py#L317-L320: replace with a call to the helper, e.g.,_validate_payload(Entity, payload, "entity").src/vouch/proposals.py#L383-L386: replace with a call to the helper, e.g.,_validate_payload(Relation, payload, "relation").♻️ Proposed helper function
from typing import Any def _validate_payload(model_cls: Any, payload: dict[str, Any], name: str) -> None: try: model_cls(**payload) except (ValidationError, TypeError) as e: raise ProposalError(f"invalid {name} payload: {e}") from e🤖 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/proposals.py` around lines 149 - 152, Extract the repeated Claim, Entity, and Relation payload validation into a shared _validate_payload helper in src/vouch/proposals.py, preserving the existing ValidationError/TypeError conversion to ProposalError and message format. Replace the blocks at src/vouch/proposals.py lines 149-152, 317-320, and 383-386 with calls using the corresponding model and name arguments: Claim/"claim", Entity/"entity", and Relation/"relation".
🤖 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.
Nitpick comments:
In `@src/vouch/proposals.py`:
- Around line 149-152: Extract the repeated Claim, Entity, and Relation payload
validation into a shared _validate_payload helper in src/vouch/proposals.py,
preserving the existing ValidationError/TypeError conversion to ProposalError
and message format. Replace the blocks at src/vouch/proposals.py lines 149-152,
317-320, and 383-386 with calls using the corresponding model and name
arguments: Claim/"claim", Entity/"entity", and Relation/"relation".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ee96314d-6e12-40f0-8478-75dd8c1f101a
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_proposals.py
what changed
propose_claim,propose_relation, andpropose_entitybuilt their payload dict and handed it straight to_file_proposalwith no check against the Claim/Relation/Entity model's own constraints (confidence: float = Field(ge=0.0, le=1.0), theRelationType/EntityTypeenums).approve()was the only place that ever caught it, via its ownClaim(**payload)construction — too late to give the proposer a usable error, and too late to keep the pending queue clean, since a proposal that fails that check can never be approved and just sits stuck there.why
Found this by exploring the CLI directly:
vouch propose-claim --confidence 1.5 ...files successfully with no error. Approving it later fails with a raw pydanticValidationError, and the proposal can never be fixed or approved — it's permanently stuck in the pending queue.fix
Validates with the same
Model(**payload)construction_payload_block_reasonandapprove()already use, so any model-level constraint is caught generically, not just confidence bounds (this also now catches an invalidRelationType/EntityTypestring at propose time, not just approve). Mirrors the propose-time validationpropose_pageandpropose_relation's existing endpoint/evidence-existence checks already do for other fields in these same functions — same established pattern, closing the one field class that slipped through it.validation commands run
Confirmed the 4 new regression tests in
tests/test_proposals.pyfail on the pre-fix code (verified via stash/restore) and pass after.Summary by CodeRabbit
Bug Fixes
Documentation