Skip to content

fix(storage): skip corrupt pages in list_pages to prevent KB-wide crashes#360

Open
RealDiligent wants to merge 1 commit into
vouchdev:testfrom
RealDiligent:fix/critical-issue-list-pages-resilience
Open

fix(storage): skip corrupt pages in list_pages to prevent KB-wide crashes#360
RealDiligent wants to merge 1 commit into
vouchdev:testfrom
RealDiligent:fix/critical-issue-list-pages-resilience

Conversation

@RealDiligent

@RealDiligent RealDiligent commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Root cause: list_pages() was the only bulk list_* path that did not use the _load_or_skip resilience pattern. A single corrupt or hand-edited pages/*.md file (missing frontmatter, bad YAML, mojibake) raised an unhandled exception and crashed vouch status, vouch lint, kb.search (substring/hybrid fallback), MCP kb.list_pages, and other core paths - while list_claims(), list_proposals(), etc. already skipped bad files with a warning.

Fix: Add _load_page_or_skip() mirroring _load_or_skip and use it in list_pages(). One regression test added alongside the existing claim test.

Impact: One bad page no longer takes down the entire KB listing surface; operators get a logged warning and the rest of the vault keeps working.

Risk / tradeoffs

  • Corrupt pages are silently omitted from listings (same behavior as corrupt claims today). Direct get_page(id) still raises for a targeted read - intentional, matches get_claim semantics.

Test plan

  • test_list_pages_skips_unreadable_file (new)
  • Mirrors existing test_list_claims_skips_unreadable_file

Summary by CodeRabbit

  • Bug Fixes
    • Page listings now continue loading even if one page file is corrupted or has invalid content.
    • Unreadable or malformed pages are skipped instead of causing the entire page list to fail.
  • Tests
    • Added coverage to confirm valid pages still appear in listings when an invalid page file is present.

list_pages() lacked the _load_or_skip resilience applied to other
list_* paths, so one bad pages/*.md file crashed status, search,
lint, and MCP tools. Skip unreadable pages with a warning instead.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: XS less than 50 changed non-doc lines labels Jul 5, 2026
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a helper function _load_page_or_skip in storage.py that catches parsing/read errors when loading page files and returns None instead of raising. KBStore.list_pages() is updated to use this helper, skipping malformed page files. A corresponding test validates this behavior.

Changes

Resilient Page Listing

Layer / File(s) Summary
Page loader helper and list_pages integration
src/vouch/storage.py
Adds _load_page_or_skip to catch YAML/validation/read errors and ValueError from _deserialize_page, returning None and logging a warning; list_pages() now filters pages through this helper instead of calling _deserialize_page directly.
Test coverage for skipped pages
tests/test_storage.py
Adds test_list_pages_skips_unreadable_file, verifying list_pages() returns only valid pages when a corrupt page file is present.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: skipping corrupt pages in list_pages to avoid broader KB crashes.
✨ 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.

@plind-junior

Copy link
Copy Markdown
Member

Is it really crashes? Attach the proof plz

@plind-junior plind-junior changed the base branch from main to test July 6, 2026 08:52
@plind-junior

Copy link
Copy Markdown
Member

With v2.0.0, we'll be moving to a database-backed approach (e.g., SQLite) instead of Markdown files. That said, I'm still curious—under what circumstances could claims or pages become corrupted in the current or future setup? Are there specific failure modes we should be proactively guarding against (e.g., concurrent writes, partial saves, file system errors, or migration mishaps)?

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

Labels

size: XS less than 50 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.

2 participants