fix: validate bundle content against Pydantic models before import (#13)#34
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds schema-aware validation for bundle members during import_check and import_apply, suppresses exception chaining in KBStore put_* handlers, changes verify_source to catch ArtifactNotFoundError, and updates session persistence and a type-check annotation. ChangesBundle validation and artifact error handling
Sequence DiagramsequenceDiagram
participant Tar as TarArchive
participant ImportCheck as import_check
participant Validator as _validate_content
participant Registry as VALIDATORS[subdir]
participant ImportApply as import_apply
participant Dest as Destination
Tar->>ImportCheck: iterate manifest-listed files
loop per member
ImportCheck->>ImportCheck: body = tar.extractfile(member).read()
ImportCheck->>Validator: _validate_content(path, body, issues)
Validator->>Registry: select validator by top-level subdir
Registry->>Validator: validate(body) / decode page
alt validation fails
Validator->>ImportCheck: append issue to issues
else validation succeeds
Validator->>ImportCheck: no issue
end
end
Tar->>ImportApply: iterate manifest-listed files
loop per member
ImportApply->>ImportApply: body = tar.extractfile(member).read()
ImportApply->>Validator: _validate_content(path, body, [])
ImportApply->>Dest: write validated body to destination
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vouch/bundle.py (2)
214-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark
import_checkas failed when validation adds issues.Line 223 hard-codes
ok=True, so schema-invalid bundles still look successful to callers even after Line 220 records validation failures.Suggested fix
return ImportCheckResult( - ok=True, bundle_id=bundle_id, + ok=not issues, bundle_id=bundle_id, new_files=new_files, conflicts=conflicts, identical=identical, issues=issues, )🤖 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/bundle.py` around lines 214 - 225, The return currently hardcodes ok=True in ImportCheckResult making schema-invalid bundles appear successful; change it so ok reflects whether any validation issues were recorded (e.g., ok = len(issues) == 0 or ok = not issues) in the import_check flow. Update the return that constructs ImportCheckResult (use the existing issues variable) so ok is false when _validate_content appended problems, keeping the rest of fields (bundle_id, new_files, conflicts, identical, issues) unchanged.
243-245:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not write members after validation has failed.
import_applyignorescheck.issues, and Line 269 discards any per-member validation failures by passing a throwaway list. Line 270 then writes the invalid bytes anyway, so the bug this PR is fixing still reproduces during apply.Suggested fix
check = import_check(kb_dir, bundle_path) + if check.issues: + raise RuntimeError(f"refusing to import invalid bundle: {'; '.join(check.issues)}") if on_conflict == "fail" and check.conflicts: raise RuntimeError(f"refusing to import: {len(check.conflicts)} conflicts") @@ dest.parent.mkdir(parents=True, exist_ok=True) body = tar.extractfile(member).read() # type: ignore[union-attr] - _validate_content(member.name, body, []) + member_issues: list[str] = [] + _validate_content(member.name, body, member_issues) + if member_issues: + skipped.append(member.name) + continue dest.write_bytes(body)Also applies to: 268-270
🤖 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/bundle.py` around lines 243 - 245, The import path currently calls import_check(kb_dir, bundle_path) but then ignores per-member validation failures (check.issues) and proceeds to write bytes in import_apply; update import_apply to respect check.issues by (1) collecting and passing the real issues list returned from import_check into the validation/writer logic instead of a throwaway list, (2) if check.issues is non-empty and on_conflict == "fail" abort the apply and raise a clear error (or propagate the RuntimeError used earlier), and (3) prevent any write of invalid members when validation failures exist; reference the functions import_check, import_apply and the check.issues field when making these changes.
🤖 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 43-51: VALIDATORS currently omits a validator for "sources", so
files under sources/* bypass schema checks; add an entry for "sources" mapping
to a validator that parses and validates using the Source model (e.g.,
"sources": lambda data: Source.model_validate(yaml.safe_load(data)) or analogous
deserializer used by other entries) so that the Source import is actually used
and malformed source artifacts are rejected during bundle processing; update
VALIDATORS to include this "sources" key referencing the Source model validator.
- Around line 31-33: Remove the unused Page import from the models import list
to satisfy Ruff and keep imports tidy: change the from .models import line to
only import Claim, Entity, Evidence, Proposal, Relation, Session, Source (remove
Page) and combine the two storage imports into a single line importing
sha256_hex and _deserialize_page from .storage; update references if any rely on
Page but note _deserialize_page is the validator used.
In `@src/vouch/verify.py`:
- Line 16: The import list in the statement importing from .storage is unsorted
and must be alphabetized to satisfy Ruff I001; reorder the imported members so
they are in alphabetical order (e.g., ArtifactNotFoundError, KBStore,
sha256_hex) in the from .storage import ... line while keeping the same imported
symbols (ArtifactNotFoundError, KBStore, sha256_hex) so the names used by
functions/classes in this module are unchanged.
---
Outside diff comments:
In `@src/vouch/bundle.py`:
- Around line 214-225: The return currently hardcodes ok=True in
ImportCheckResult making schema-invalid bundles appear successful; change it so
ok reflects whether any validation issues were recorded (e.g., ok = len(issues)
== 0 or ok = not issues) in the import_check flow. Update the return that
constructs ImportCheckResult (use the existing issues variable) so ok is false
when _validate_content appended problems, keeping the rest of fields (bundle_id,
new_files, conflicts, identical, issues) unchanged.
- Around line 243-245: The import path currently calls import_check(kb_dir,
bundle_path) but then ignores per-member validation failures (check.issues) and
proceeds to write bytes in import_apply; update import_apply to respect
check.issues by (1) collecting and passing the real issues list returned from
import_check into the validation/writer logic instead of a throwaway list, (2)
if check.issues is non-empty and on_conflict == "fail" abort the apply and raise
a clear error (or propagate the RuntimeError used earlier), and (3) prevent any
write of invalid members when validation failures exist; reference the functions
import_check, import_apply and the check.issues field when making these changes.
🪄 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: d0227838-0598-423f-b712-1754cf82fc8a
📒 Files selected for processing (2)
src/vouch/bundle.pysrc/vouch/verify.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vouch/sessions.py (1)
17-17: ⚡ Quick winKeep session persistence behind
KBStore's public API.Importing
_yaml_dumpand callingstore._session_path(...)couplesession_endto storage internals, so any later change to write semantics or layout now has to be duplicated here too. Please expose an overwrite/update method onKBStoreand call that instead.♻️ Proposed direction
-from .storage import KBStore, _yaml_dump +from .storage import KBStore ... - path = store._session_path(sess.id) - path.write_text(_yaml_dump(sess.model_dump(mode="json"))) + store.update_session(sess)Then implement
KBStore.update_session(...)in.storageusing the canonical session-write path.Also applies to: 47-48
🤖 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/sessions.py` at line 17, session_end currently imports and uses the internal _yaml_dump and store._session_path, coupling session persistence to storage internals; add a public KBStore.update_session(session_id, data, mode=...) (or similar signature) in storage and replace calls to _yaml_dump and store._session_path inside session_end with a single call to store.update_session(...) so the KBStore implementation owns the session write semantics; update any other places that use store._session_path/_yaml_dump (e.g., lines 47-48) to call the new KBStore.update_session API instead.
🤖 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/sessions.py`:
- Line 17: session_end currently imports and uses the internal _yaml_dump and
store._session_path, coupling session persistence to storage internals; add a
public KBStore.update_session(session_id, data, mode=...) (or similar signature)
in storage and replace calls to _yaml_dump and store._session_path inside
session_end with a single call to store.update_session(...) so the KBStore
implementation owns the session write semantics; update any other places that
use store._session_path/_yaml_dump (e.g., lines 47-48) to call the new
KBStore.update_session API instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2661a456-0377-4627-ba85-88cc81747957
📒 Files selected for processing (1)
src/vouch/sessions.py
Fix: import_check ok=not issues, import_apply block bad writes
_validate_content now only processes .yaml, .yml, .md files to avoid failing on raw binary data like sources/*/content
Collaborator
|
Hey — I think this one's already done? The same fix is on Could you rebase and see if there's anything left? If the diff is empty just close it out and point at Thanks! |
Collaborator
|
Fix the conflict or it will be closed in 12hr |
# Conflicts: # src/vouch/bundle.py
plind-junior
added a commit
that referenced
this pull request
May 22, 2026
fix: validate bundle content against Pydantic models before import (#13)
This was referenced May 25, 2026
This was referenced May 25, 2026
This was referenced Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
import_applywrote raw bytes to committed artifact directories without checking file content against the Pydantic models. A crafted bundle could inject malformed YAML or schema-violating artifacts that poison all subsequent list/read operations.Changes
import_checknow validates content — issues surface before applyimport_applyvalidates content before writing to diskFixes #13
Summary by CodeRabbit
New Features
Bug Fixes
Chores