fix(vault-sync): surface deleted-citation error as VaultSyncError#548
Open
boskodev790 wants to merge 1 commit into
Open
fix(vault-sync): surface deleted-citation error as VaultSyncError#548boskodev790 wants to merge 1 commit into
boskodev790 wants to merge 1 commit into
Conversation
sync_vault's forward pass caught ArtifactNotFoundError to turn a deleted-citation conflict into a clean VaultSyncError, but propose_page validates cited ids and converts the miss to ProposalError before it surfaces. the two are disjoint hierarchies (ProposalError(RuntimeError) vs ArtifactNotFoundError(KeyError)), so the handler was dead and the ProposalError escaped as an uncaught traceback — bypassing the CLI's own `except VaultSyncError` renderer, which turns domain errors into a one-line message. catch ProposalError alongside ArtifactNotFoundError so a vault edit to a page whose cited claim/entity/source was deleted lands as the documented VaultSyncError. driving `vouch sync` on that scenario now prints "Error: vault edit references unknown artifact: unknown claim id: ..." instead of a python traceback. this does not keep watch_vault running — that loop catches only KeyboardInterrupt and still ends on either exception; the fix is about the surfaced error type, not loop survival.
github-actions
Bot
added
docs
WalkthroughThis change updates forward vault synchronization to catch ChangesVault sync error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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`:
- Around line 276-282: Move the `sync_vault`/`ProposalError` changelog bullet
from the `## [1.4.0] — 2026-07-17` section into the existing `[Unreleased]`
section, preserving its wording and formatting.
In `@src/vouch/vault_sync.py`:
- Around line 468-476: Update the inline comment block near the vault edit
conflict handler to use lowercase prose throughout, including “a vault edit,”
“that’s,” and “catch both,” while preserving exception and type names such as
ArtifactNotFoundError, ProposalError, and VaultSyncError.
- Around line 467-476: The VaultSync exception handler must not label every
ProposalError as an unknown artifact. Update the handling around propose_page()
to distinguish missing claim/entity/source errors from other proposal validation
failures, or use a neutral proposal-rejected message for ProposalError while
preserving the documented VaultSyncError conversion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39b3b38a-c780-4c02-8909-de7a8f38840f
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/vault_sync.pytests/test_vault_sync.py
Comment on lines
+276
to
+282
| - `sync_vault` catches the `ProposalError` that `propose_page` raises for a | ||
| deleted citation, not just `ArtifactNotFoundError`. a vault edit to a page | ||
| whose cited claim/entity/source was since removed hit propose_page's id | ||
| validation, which converts the miss to `ProposalError` — so the | ||
| `except ArtifactNotFoundError` handler never fired and the error escaped as | ||
| an uncaught traceback, bypassing the CLI's `except VaultSyncError` renderer. | ||
| it now surfaces as the intended one-line `VaultSyncError` (#547). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
move this entry under [Unreleased]
This is a user-visible cli error change, but it is currently recorded under ## [1.4.0] — 2026-07-17. Move the bullet to the existing [Unreleased] section.
as per coding guidelines: user-visible changes must be recorded in CHANGELOG.md under [Unreleased] in the same pr; as per path instructions: do not silently omit user-visible changes.
🤖 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 `@CHANGELOG.md` around lines 276 - 282, Move the `sync_vault`/`ProposalError`
changelog bullet from the `## [1.4.0] — 2026-07-17` section into the existing
`[Unreleased]` section, preserving its wording and formatting.
Sources: Coding guidelines, Path instructions
Comment on lines
+467
to
+476
| except (ArtifactNotFoundError, ProposalError) as e: | ||
| # A vault edit referenced a claim/entity/source that no longer | ||
| # exists in the KB. That's a *real* conflict the user has to | ||
| # resolve in Obsidian, not a vouch bug; surface it cleanly. | ||
| # propose_page validates those ids and converts the miss to a | ||
| # ProposalError ("unknown claim/entity/source id"), not the raw | ||
| # ArtifactNotFoundError this used to catch — so the handler was | ||
| # dead and the ProposalError escaped as an uncaught traceback, | ||
| # bypassing the CLI's `except VaultSyncError` renderer. Catch | ||
| # both so it surfaces as the documented VaultSyncError. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
do not classify every ProposalError as an unknown artifact
propose_page() in src/vouch/proposals.py also raises ProposalError for empty titles and page-kind validation failures. Those errors now receive the misleading unknown artifact message. Distinguish missing-artifact causes from other proposal failures, or use a neutral proposal-rejected message.
🤖 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/vault_sync.py` around lines 467 - 476, The VaultSync exception
handler must not label every ProposalError as an unknown artifact. Update the
handling around propose_page() to distinguish missing claim/entity/source errors
from other proposal validation failures, or use a neutral proposal-rejected
message for ProposalError while preserving the documented VaultSyncError
conversion.
Comment on lines
468
to
+476
| # A vault edit referenced a claim/entity/source that no longer | ||
| # exists in the KB. That's a *real* conflict the user has to | ||
| # resolve in Obsidian, not a vouch bug; surface it cleanly. | ||
| # propose_page validates those ids and converts the miss to a | ||
| # ProposalError ("unknown claim/entity/source id"), not the raw | ||
| # ArtifactNotFoundError this used to catch — so the handler was | ||
| # dead and the ProposalError escaped as an uncaught traceback, | ||
| # bypassing the CLI's `except VaultSyncError` renderer. Catch | ||
| # both so it surfaces as the documented VaultSyncError. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
keep the new inline comment prose lowercase
The added comment block uses sentence case (A vault edit, That's, Catch both). Lowercase the prose while preserving exception and type names.
as per path instructions: use lowercase prose in comments and review notes.
🤖 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/vault_sync.py` around lines 468 - 476, Update the inline comment
block near the vault edit conflict handler to use lowercase prose throughout,
including “a vault edit,” “that’s,” and “catch both,” while preserving exception
and type names such as ArtifactNotFoundError, ProposalError, and VaultSyncError.
Source: Path instructions
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.
What changed
sync_vault's forward-pass handler catchesProposalErroralongsideArtifactNotFoundError, so a vault edit to a page whose cited claim/entity/source was deleted surfaces as the documented one-lineVaultSyncErrorinstead of an uncaught traceback.Why
fixes #547.
vault_to_kbfiles the edit viapropose_page, which validates each cited id and converts a miss toProposalError(proposals.py:299-300), not the rawArtifactNotFoundErrorthe handler caught. The two are disjoint hierarchies (ProposalError(RuntimeError)vsArtifactNotFoundError(KeyError)), so theexceptnever fired — the handler was dead code and theProposalErrorpropagated uncaught, bypassing the CLI's ownexcept VaultSyncErrorrenderer (cli.py:5021).I drove
vouch syncon the scenario both ways:vouch.proposals.ProposalError: unknown claim id: c1Error: vault edit references unknown artifact: unknown claim id: c1Honest scope note: this does not keep
watch_vaultrunning — that loop catches onlyKeyboardInterrupt, so it ends on either exception. The fix is specifically about the surfaced error type (documentedVaultSyncErrorvs uncaughtProposalError) and about making the dead handler live, not loop survival.What might break
nothing on disk: no file moves, no field changes shape, no
kb.*method behaves differently.sync_vaultalready had thisexcept; the change only widens the exception tuple. A vault edit citing a deleted artifact that previously produced a traceback now produces a cleanVaultSyncError(the documented intent). No existing passing path changes — verified the happy path still proposes normally and the other 25 vault_sync tests pass unchanged.VEP
not a surface change — no
kb.*method, object model, on-disk layout, bundle format or audit-log shape touched.vault_sync.pyis non-core. no VEP.Tests
make checkpasses locally (lint + mypy + pytest) — 1679 passed, 50 skipped; ruff clean; mypy clean across 104 source filesCHANGELOG.mdupdated under## [Unreleased]test_sync_vault_surfaces_deleted_citation_as_vault_sync_error— verified failing ontestwithout the src change (the rawProposalErrorescapes); passing with it. End-to-end drive also confirmed the fix covers entity and source citations, not just claims, and doesn't over-catch a legitimate edit.no ui surface touched, so no screenshots.
Summary by CodeRabbit
Bug Fixes
Tests