Skip to content

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

Open
wondercreatemaster wants to merge 2 commits into
vouchdev:testfrom
wondercreatemaster:fix/vault-sync-deleted-citation-547
Open

fix(vault-sync): surface deleted-citation as VaultSyncError#564
wondercreatemaster wants to merge 2 commits into
vouchdev:testfrom
wondercreatemaster:fix/vault-sync-deleted-citation-547

Conversation

@wondercreatemaster

@wondercreatemaster wondercreatemaster commented Jul 27, 2026

Copy link
Copy Markdown

What changed

sync_vault's forward-pass handler now catches the ProposalError that propose_page raises for a deleted citation (missing claim/entity/source id, with ArtifactNotFoundError as __cause__), and converts it to the documented VaultSyncError. other proposal failures get a neutral vault edit rejected message so they are not mislabelled as unknown artifacts.

Why

Fixes #547. propose_page converts lookup misses to ProposalError, so the old except ArtifactNotFoundError handler was dead and vouch sync printed an uncaught traceback instead of the CLI's one-line Error: renderer. prior attempt #548 was closed after CodeRabbit requested changes (changelog under the wrong section; every ProposalError labelled as unknown artifact; comment casing) — this addresses all three.

What might break

nothing on disk. existing .vouch/ layouts and kb.* methods are unchanged. only the exception type surfaced by sync_vault / vouch sync --direction forward for these failure modes changes (traceback → clean VaultSyncError).

VEP

n/a — bugfix, no surface change.

Tests

  • make check passes locally (lint + mypy + pytest) — 1766 passed, 50 skipped
  • New / changed behaviour has a test
  • CHANGELOG.md updated under ## [Unreleased]

regression coverage:

  • deleted citation → VaultSyncError matching unknown artifact
  • non-artifact ProposalErrorvault edit rejected, not unknown artifact
  • CLI path prints Error: with no traceback

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes

    • Improved forward vault synchronization to correctly classify failures caused by deleted citations.
    • Other rejected page edits are now surfaced as a consistent “vault edit rejected” error.
    • CLI sync failures now show a single-line Error: message without tracebacks, with clearer “unknown artifact” details.
  • Tests

    • Added regression tests for deleted citation sync behavior, ensuring correct error text and no traceback leakage.
    • Added CLI integration coverage for clean error output on deleted-citation scenarios.

propose_page converts missing claim/entity/source ids to ProposalError,
so the except ArtifactNotFoundError handler was dead and the error
escaped as a traceback past the CLI renderer. catch ProposalError when
its cause is ArtifactNotFoundError; other proposal failures get a
neutral vault-edit-rejected message so they are not mislabelled.
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance sync sync, vault mirror, and diff flows tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 27, 2026
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8952af40-8fbc-4595-80d5-b6660cabc087

📥 Commits

Reviewing files that changed from the base of the PR and between 01760d4 and 685fc25.

📒 Files selected for processing (1)
  • tests/test_vault_sync.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_vault_sync.py

Walkthrough

Forward vault synchronization now wraps ProposalError failures as categorized VaultSyncError exceptions. Tests cover deleted citations, other proposal rejections, and clean CLI output, while the changelog documents the fix.

Changes

Vault sync error handling

Layer / File(s) Summary
Classify forward-sync proposal failures
src/vouch/vault_sync.py
sync_vault distinguishes deleted-artifact proposal failures from other rejected vault edits and raises the corresponding VaultSyncError.
Validate exception and CLI behavior
tests/test_vault_sync.py, CHANGELOG.md
Tests cover exception classification and traceback-free CLI output; the changelog records the correction.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • vouchdev/vouch#548: Updates forward-sync ProposalError wrapping with related regression tests.

Suggested labels: cli

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix to vault sync error handling for deleted citations.
Linked Issues check ✅ Passed The changes address #547 by catching proposal failures and surfacing them as clean VaultSyncError output.
Out of Scope Changes check ✅ Passed The added tests and changelog note support the vault-sync fix and do not appear unrelated to the issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@tests/test_vault_sync.py`:
- Around line 343-367: Strengthen test_cli_sync_deleted_citation_is_clean_error
by asserting the CLI error rendering is exactly one line, not merely
traceback-free. Normalize result.output as needed and compare it against the
documented expected Error: message while preserving the existing unknown
artifact and unknown claim id checks.
🪄 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: 815951f9-f772-4ea8-978b-1f3adf88bcfb

📥 Commits

Reviewing files that changed from the base of the PR and between 0e45e38 and 01760d4.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • src/vouch/vault_sync.py
  • tests/test_vault_sync.py

Comment thread tests/test_vault_sync.py
strengthen the CLI regression to the documented Error: contract from
vouchdev#547 so a multiline non-traceback rendering can no longer pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs documentation, specs, examples, and repo guidance size: S 50-199 changed non-doc lines sync sync, vault mirror, and diff flows tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant