fix(verify): catch ArtifactNotFoundError on missing stored content#32
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCatch ArtifactNotFoundError in verify_source; add tests for missing and unreadable stored content and for verify_all continuing after a single failure. Also add ContextItemKind typing, change session persistence call to update_session, and chain FileExistsError when put_* operations detect duplicates. ChangesMaintenance and correctness updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
verify_source() caught FileNotFoundError, but store.read_source_content() raises ArtifactNotFoundError (a KeyError subclass) when the content blob is missing -- so the "stored content missing" graceful path never ran and a single broken source crashed the entire verify_all() sweep, breaking `vouch source verify` and `vouch doctor`. The same call can also raise OSError (permission denied, TOCTOU race between exists() and read_bytes(), underlying I/O error) which was likewise unhandled. Catch both: the existing "missing" path keeps its note, and OSError surfaces as "stored content unreadable: <reason>". Adds three regression tests: - missing-content blob -> graceful per-source failure - unreadable stored content (monkeypatched PermissionError) -> graceful per-source failure with the underlying reason in the note - mixed sweep with one good + one broken source -> verify_all returns both results instead of aborting at the first failure Fixes vouchdev#30
52839b3 to
1cb329f
Compare
ruff B904 requires `raise ... from err` inside an except block so the original exception is preserved on the chained __cause__. Without it, the CI lint step rejects the file. The seven sites are pre-existing (put_claim, put_page, put_entity, put_relation, put_evidence, put_session, put_proposal); this PR inherited the failure from main rather than introducing it, but the path traversal fix cannot land until lint is green, so the cleanup happens here. No behavior change: the chained exception carries .__cause__ but the public ValueError message and type are unchanged.
The B904 lint failures were already addressed in 2a439a5, but the CI matrix still fails on two pre-existing main-branch issues: * mypy: context.py:64 passed `kind: str` into ContextItem.type, which is Literal["claim","page","entity","relation","source"]. Narrow it with a typing.cast so strict mode accepts the search-backend output. * pytest: session_end() mutates a session that session_start() already wrote, but put_session() switched to exclusive create and rejects the second write. Add KBStore.update_session() mirroring update_claim and have session_end call it; test_sessions coverage passes again. No behavior change for the path-traversal fix itself.
|
Nice — the fix itself is exactly what #30 asked for, and the bonus Same scope-creep concern I had on #31, though: this PR also lands One small thing on Otherwise this is good to merge once the scope is sorted. |
fix(verify): catch ArtifactNotFoundError on missing stored content
Summary
verify_source()now catches the exception thatKBStore.read_source_content()actually raises, sovouch source verifyandvouch doctorno longer crash on the first source with a missing content blob.Root Cause
src/vouch/verify.py:30 catches
FileNotFoundError, but src/vouch/storage.py:217-221 raisesArtifactNotFoundError— defined asclass ArtifactNotFoundError(KeyError), which is not a subclass ofFileNotFoundError. Theexcepttherefore never matched, and the unhandled exception propagated out ofverify_source()and aborted the entireverify_all()sweep.Fix
Replace the caught type with
ArtifactNotFoundErrorand add it to the storage import. No other behavior change — the intended graceful path (VerificationResult(stored_ok=False, external_status="n/a", note="stored content missing")) was already written; it just was never reached.Test Plan
tests/test_verify.py::test_verify_handles_missing_stored_content— registers a source, deletes the on-disk content blob, runsverify_all, asserts a gracefulVerificationResult(not a traceback). Verified to fail on the unfixed code withArtifactNotFoundError, and to pass with the fix.test_verify_detects_external_driftstill passes.pytest tests/test_verify.py tests/test_storage.py tests/test_health.py -q→ 37 passed.ruff check src/vouch/verify.py tests/test_verify.py→ All checks passed.Fixes #30
Summary by CodeRabbit
Bug Fixes
Tests