From 27c51135e80ac5873330dd94bd523d6595271088 Mon Sep 17 00:00:00 2001 From: kai392 Date: Fri, 31 Jul 2026 04:46:17 +0800 Subject: [PATCH] fix: resolve silent page overwrite on colliding propose_page approve Co-authored-by: Cursor --- src/vouch/proposals.py | 59 ++++++++++++++++++++++++++++++----------- src/vouch/vault_sync.py | 3 +++ tests/test_storage.py | 50 +++++++++++++++++++++++++++++++++- 3 files changed, 95 insertions(+), 17 deletions(-) diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index d1ca8464..79bc3109 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -78,6 +78,9 @@ def missing_claim_refs(store: KBStore, proposal: Proposal) -> list[str]: EXPIRE_REASON = "expired" EXPIRE_ACTOR = "vouch-expire" ADMISSION_ACTOR = "vouch-admission" +# Payload flag stamped by propose_page(update_existing=True). Popped before +# Page model validation so it never reaches the durable artifact. +_UPDATE_EXISTING_KEY = "_update_existing" _DEFAULT_EXPIRE_PENDING_DAYS = 90 @@ -334,6 +337,7 @@ def propose_page( session_id: str | None = None, dry_run: bool = False, scope: dict[str, Any] | str | None = None, + update_existing: bool = False, ) -> Proposal: if not title.strip(): raise ProposalError("page title is empty") @@ -369,8 +373,20 @@ def propose_page( ) except PageKindError as e: raise ProposalError(str(e)) from e - payload = { - "id": slug_hint or _slugify(title), + page_id = slug_hint or _slugify(title) + # Opt-in update path for vault_to_kb (#219). Without this flag, approve() + # refuses an existing page id the same way it refuses claim collisions — + # otherwise a colliding title/slug (or a malicious slug_hint) silently + # overwrites durable content via update_page. + if update_existing: + try: + store.get_page(page_id) + except ArtifactNotFoundError as e: + raise ProposalError( + f"cannot update: page {page_id} does not exist" + ) from e + payload: dict[str, Any] = { + "id": page_id, "title": title.strip(), "body": body, "type": page_type, @@ -380,6 +396,8 @@ def propose_page( "tags": tags or [], "metadata": meta, } + if update_existing: + payload[_UPDATE_EXISTING_KEY] = True _stamp_scope(store, payload, scope) return _file_proposal( store, kind=ProposalKind.PAGE, payload=payload, @@ -713,7 +731,8 @@ def auto_approve_pending( closed instead of piling up; pages, entities and relations are approved unless something still blocks them. What stays pending is exactly the human-call residue: protected page kinds, pages with dead claim - references, an id already durable with different content, and DELETE + references, an id already durable (pages included — colliding page + proposals no longer overwrite via update_page), and DELETE proposals (retracting durable knowledge is never drained mechanically). Without trusted-agent this falls back to the receipt drain @@ -794,10 +813,17 @@ def _payload_block_reason( except ValueError as e: return str(e) elif proposal.kind == ProposalKind.PAGE: + page_payload = dict(payload) + is_update = bool(page_payload.pop(_UPDATE_EXISTING_KEY, False)) try: - page = Page(**payload) + page = Page(**page_payload) except (ValidationError, TypeError) as e: return f"invalid page payload: {e}" + if is_update: + try: + store.get_page(page.id) + except ArtifactNotFoundError: + return f"cannot update: page {page.id} does not exist" if not skip_dead_claim_refs: for cid in page.claims: if not store._claim_path(cid).exists(): @@ -882,10 +908,14 @@ def approve( # Refuse to overwrite an existing artifact. Without this guard a retry # after a crash between put_() and move_proposal_to_decided() would # silently rewrite the artifact with new approved_by / created_at metadata. - # Exception: PAGE proposals may legitimately target an existing page when - # filed by vault_to_kb (vault edit flow) — the approve path handles that - # via update_page rather than put_page. - if proposal.kind not in (ProposalKind.PAGE, ProposalKind.DELETE): + # PAGE updates are opt-in via propose_page(update_existing=True) — used by + # vault_to_kb (#219). A bare colliding propose_page must not take the + # update_page path or durable content (and scope) is silently wiped. + is_page_update = ( + proposal.kind == ProposalKind.PAGE + and bool(payload.get(_UPDATE_EXISTING_KEY)) + ) + if proposal.kind != ProposalKind.DELETE and not is_page_update: _ensure_no_existing_artifact(store, proposal.kind, payload["id"]) result: Claim | Page | Entity | Relation if proposal.kind == ProposalKind.CLAIM: @@ -914,6 +944,7 @@ def approve( if isinstance(payload.get("body"), str): payload["body"] = strip_claim_markers(payload["body"], missing) dropped_claims = missing + is_update = bool(payload.pop(_UPDATE_EXISTING_KEY, False)) page = Page(**payload) # Re-validate the kind at the gate: config may have tightened (or a # kind been removed) between propose and approve. Built-in kinds pass @@ -927,15 +958,11 @@ def approve( ) except PageKindError as e: raise ProposalError(str(e)) from e - # Vault-edit proposals use slug_hint=page_id so the payload id matches - # an existing page. In that case update rather than create so the - # approve path doesn't raise "page already exists" for every normal - # vault edit. For new pages (no existing artifact) put_page is used - # as before. - try: - store.get_page(page.id) + # Opt-in update (vault edit) vs create. Existence for updates was + # already checked in _payload_block_reason / the collision guard. + if is_update: store.update_page(page) - except ArtifactNotFoundError: + else: store.put_page(page) with index_db.open_db(store.kb_dir) as conn: index_db.index_page( diff --git a/src/vouch/vault_sync.py b/src/vouch/vault_sync.py index 373a174a..ba86db7f 100644 --- a/src/vouch/vault_sync.py +++ b/src/vouch/vault_sync.py @@ -441,6 +441,9 @@ def vault_to_kb( proposed_by=actor, tags=list(edited.tags), slug_hint=page_id, + # Opt-in update: without this, approve() refuses the existing id + # (same collision guard as claims) and a vault edit would fail. + update_existing=True, # A vault edit is a body edit, never a scope change: carry the # durable page's own scope through, or the propose gate would # restamp it with the KB default (silently widening/narrowing diff --git a/tests/test_storage.py b/tests/test_storage.py index 5438b5bf..7b139a99 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -640,7 +640,7 @@ def test_approve_page_update_rejects_stale_claim_ref(store: KBStore) -> None: ) pr = propose_page( store, title="T", body="updated", claim_ids=["c1"], - proposed_by="vault-sync", slug_hint="p1", + proposed_by="vault-sync", slug_hint="p1", update_existing=True, ) (store.kb_dir / "claims" / "c1.yaml").unlink() # DeadClaimRefsError (a ProposalError) is the specific refusal: the @@ -844,6 +844,54 @@ def test_propose_page_round_trip_through_approval(store: KBStore) -> None: assert store.get_page(artifact.id).body == "body" +def test_approve_page_collision_does_not_overwrite(store: KBStore) -> None: + # Colliding title/slug must refuse, not silently update_page the durable + # artifact. Before the fix, PAGE was exempt from the collision guard and + # any matching id took the vault-edit update path. + store.put_page(Page(id="deploy-workflow", title="Deploy Workflow", + body="ORIGINAL")) + pr = propose_page( + store, title="Deploy Workflow", body="WIPED", proposed_by="agent", + ) + with pytest.raises(ProposalError, match="already exists"): + approve(store, pr.id, approved_by="reviewer") + assert store.get_page("deploy-workflow").body == "ORIGINAL" + + +def test_approve_page_update_existing_opt_in(store: KBStore) -> None: + # Explicit update_existing=True (vault_to_kb path) still updates in place. + store.put_page(Page(id="alpha-page", title="Alpha", body="old")) + pr = propose_page( + store, title="Alpha", body="new", proposed_by="vault-sync", + slug_hint="alpha-page", update_existing=True, + ) + approve(store, pr.id, approved_by="reviewer") + assert store.get_page("alpha-page").body == "new" + + +def test_approve_page_update_fails_if_target_deleted(store: KBStore) -> None: + # Page removed between propose and approve: update must refuse, not create. + store.put_page(Page(id="alpha-page", title="Alpha", body="old")) + pr = propose_page( + store, title="Alpha", body="new", proposed_by="vault-sync", + slug_hint="alpha-page", update_existing=True, + ) + (store.kb_dir / "pages" / "alpha-page.md").unlink() + reason = check_approvable(store, pr.id, approved_by="reviewer") + assert reason is not None and "does not exist" in reason + with pytest.raises(ProposalError, match="does not exist"): + approve(store, pr.id, approved_by="reviewer") + assert not (store.kb_dir / "pages" / "alpha-page.md").exists() + + +def test_propose_page_update_existing_requires_target(store: KBStore) -> None: + with pytest.raises(ProposalError, match="does not exist"): + propose_page( + store, title="Ghost", body="b", proposed_by="vault-sync", + slug_hint="no-such-page", update_existing=True, + ) + + def test_propose_page_rejects_unknown_claim_ref(store: KBStore) -> None: with pytest.raises(ProposalError, match="unknown claim id"): propose_page(