Skip to content

fix(vault-sync): surface deleted-citation error as VaultSyncError #548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
boskodev790 wants to merge 1 commit into
base: test
Choose a base branch
from
+36 −2
Open

fix(vault-sync): surface deleted-citation error as VaultSyncError #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,13 @@ All notable changes to vouch are documented here. Format follows
in config.yaml (#476).

### Fixed
- `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).
Comment on lines +276 to +282

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

- approve/reject/expire record the audit event *before* moving the
proposal to decided/. a crash between the two used to leave a durable
decision with no authoritative history; it now leaves a pending
Expand Down
10 changes: 8 additions & 2 deletions src/vouch/vault_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
ClaimStatus,
PageStatus,
)
from .proposals import propose_page
from .proposals import ProposalError, propose_page
from .storage import (
ArtifactNotFoundError,
KBStore,
Expand Down Expand Up @@ -464,10 +464,16 @@ def sync_vault(
if direction in {"forward", "both"}:
try:
r = vault_to_kb(store, vault_dir, actor=actor)
except ArtifactNotFoundError as e:
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.
Comment on lines +467 to +476

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

raise VaultSyncError(f"vault edit references unknown artifact: {e}") from e
combined.pages_proposed.extend(r.pages_proposed)
combined.pages_skipped_unchanged.extend(r.pages_skipped_unchanged)
Expand Down
21 changes: 21 additions & 0 deletions tests/test_vault_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,27 @@ def test_sync_vault_rejects_missing_vault(store: KBStore, tmp_path: Path) -> Non
sync_vault(store, tmp_path / "does-not-exist")


def test_sync_vault_surfaces_deleted_citation_as_vault_sync_error(
store: KBStore, vault: Path,
) -> None:
"""A vault edit to a page whose cited claim was since deleted must
surface as a clean VaultSyncError, not the raw ProposalError that
propose_page raises for an unknown id — otherwise it escapes the dead
`except ArtifactNotFoundError` handler as an uncaught traceback,
bypassing the CLI's VaultSyncError renderer. Fixture cites `alpha-claim`."""
kb_to_vault(store, vault)
mirror = vault / VAULT_DIR / "pages" / "alpha-page.md"
mirror.write_text(
mirror.read_text(encoding="utf-8").replace("Original body.", "Edited."),
encoding="utf-8",
)
# Delete the cited claim so propose_page's id validation fails.
(store.kb_dir / "claims" / "alpha-claim.yaml").unlink()

with pytest.raises(VaultSyncError, match="unknown artifact"):
sync_vault(store, vault, direction="forward")


# --- CLI surface ----------------------------------------------------------


Expand Down
Loading