Skip to content

fix(vault-sync): key pending-proposal dedup on edit content, not page id - #707

Merged
plind-junior merged 4 commits into
vouchdev:testfrom
philluiz2323:fix/vault-sync-second-edit-clobbered
Jul 31, 2026
Merged

fix(vault-sync): key pending-proposal dedup on edit content, not page id#707
plind-junior merged 4 commits into
vouchdev:testfrom
philluiz2323:fix/vault-sync-second-edit-clobbered

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

What changed

_has_pending_page_proposal's dedup check for pending page proposals now
keys on source_id — the content-address (sha256) of the whole vault-edit
mirror file — instead of comparing raw body text (which the call site in
vault_to_kb never actually passed, so the guard degraded to id-only).

Why

The dedup guard only checked whether any pending proposal targeted a
page id, regardless of what edit it contained. So a second, distinct edit
made to a vault page while the first edit's proposal was still pending
was silently skipped instead of filing its own proposal. The second edit
was never captured anywhere — not in a proposal, not in the KB — and the
next backward sync pass then overwrote the vault mirror with the KB's
still-just-the-first-edit content, erasing the second edit with no trace
and no error.

Confirmed with a repro: edit a mirrored page (edit A), run
vault_to_kb (proposal correctly filed), edit the same page again with
unrelated content (edit B) before approving edit A's proposal, run
vault_to_kb again — edit B never gets a proposal
(pages_proposed: [], pages_skipped_unchanged: [...]) and is gone.

Fixes #706

What might break

Nothing for users with an existing .vouch/ directory — no on-disk
shape, kb.* method, or object model change. Behaviorally: a second,
distinct edit to an already-pending page now files its own proposal
instead of being silently dropped. The original dedup guarantee this
function exists for (#219 — re-running sync on an unchanged edit must
not file a duplicate) still holds, since an unchanged edit has the same
source_id as the already-pending proposal.

VEP

Not applicable — no object model, kb.* method, on-disk layout, bundle
format, or audit-log shape change. A dedup-key correctness fix inside
the vault sync internals.

Tests

  • Local make check-equivalent: ruff clean (src + tests); mypy
    clean on vault_sync.py; all tests/test_vault_sync.py cases pass
    (32 pre-existing + 1 new)
  • New / changed behaviour has a test —
    test_vault_to_kb_files_new_proposal_for_second_distinct_edit
  • CHANGELOG.md updated under ## [Unreleased]

_has_pending_page_proposal only checked whether *any* pending proposal
targeted a page id, so a second, distinct edit made to a vault page
while the first edit's proposal was still pending got silently skipped
instead of filing its own proposal - vault_to_kb never passed the
existing body kwarg, so the guard degraded to id-only. the second edit
was never captured anywhere: not in a proposal, not in the kb, and the
next backward sync pass then overwrote the vault mirror with the kb's
still-just-the-first-edit content, erasing the second edit with no
trace.

key the dedup check on source_id - the content-address (sha256) of the
whole mirror file, the same fingerprint put_source assigns - instead of
comparing raw body text. a second, different edit now correctly files
its own proposal alongside the first instead of being coalesced into it.

Fixes vouchdev#706
@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 31, 2026
@plind-junior

Copy link
Copy Markdown
Member

the diagnosis is right and the content-address key is the correct fix — body was never passed from the call site, so the guard had quietly degraded to id-only, and keying on the source id fingerprints the whole edit (title, type, tags, claims, entities, body) rather than one field. worth noting the existing test_vault_to_kb_deduplicates_pending_proposals is what proves the #219 guarantee still holds, since an unchanged edit re-derives the same current_hash; if the hash and the filed sources entry ever disagreed, that test would go red rather than this behaviour silently regressing.

the thing i would want answered before merge is what a reviewer is supposed to do with two pending proposals for one page. vault_sync passes update_existing=True (vault_sync.py:446), so both proposals are approvable — which is good, nothing is stuck — but it also means approval order decides the outcome, and nothing in the proposal carries an ordering marker. approve edit B then edit A and the page ends up holding A, a silent revert to the older edit, with both proposals showing APPROVED in decided/. under the old id-only guard that was impossible because there was only ever one proposal.

so the fix trades a silent clobber for an order-dependent one. that is still a clear improvement — the second edit is at least captured now, and a reviewer draining the queue in order gets the right answer — but it is worth either a note in the docstring that these must be approved oldest-first, or having the second proposal reference the one it supersedes. a test that approves out of order and asserts the resulting body would pin whichever behaviour you decide is correct.

smaller: the rewritten docstring ends on "Without this guard, running vault_to_kb twice before the first proposal is approved files duplicate proposals for the same edit" — that sentence describes the absence of the page-id check (the original #219 fix), not the absence of the source_id refinement this PR adds. as written it reads as if dropping source_id would cause duplicates, when dropping it causes the opposite. worth splitting the two guards into two sentences.

philluiz2323 and others added 3 commits July 31, 2026 00:45
@plind-junior
plind-junior merged commit d6ef0df into vouchdev:test Jul 31, 2026
11 checks passed
@github-actions github-actions Bot added the ci: passing ci is green label Jul 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

diff coverage: n/a — this PR changes no python under src/vouch/, so there is nothing for the gate to measure.

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

Labels

ci: passing ci is green 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.

vault sync silently drops a second vault edit made before the first edit is approved

2 participants