diff --git a/CHANGELOG.md b/CHANGELOG.md index 4458b62e..2e3c9ab3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,13 @@ All notable changes to vouch are documented here. Format follows `contradicts` reference and flipped the claim to `contested` with no actual counterparty; it now raises `LifecycleError`, mirroring the existing guard in `supersede()`. +- `check_approvable()` now catches an id collision with an existing + artifact, mirroring the guard `approve()` already applies. previously + the batch precheck could report a claim/entity/relation proposal as + approvable when its id matched an artifact that already existed, so + `vouch approve a b` could pass the precheck for the whole batch and + then fail partway through on the collision, breaking the documented + all-or-nothing guarantee. ### Added - `kb.list_skills` / `kb.get_skill` — agents can enumerate the Claude Code diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index e66c1298..48795999 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -494,6 +494,14 @@ def check_approvable( block = _approval_block_reason(store, proposal, approved_by) if block: return block + # Mirror approve()'s existing-artifact guard so the batch precheck can't + # say "approvable" for a proposal that approve() will then reject. + if proposal.kind not in (ProposalKind.PAGE, ProposalKind.DELETE): + existing = _existing_artifact_block_reason( + store, proposal.kind, proposal.payload["id"] + ) + if existing: + return existing return _payload_block_reason(store, proposal) @@ -878,21 +886,29 @@ def _approve_delete( return artifact -def _ensure_no_existing_artifact( +def _existing_artifact_block_reason( store: KBStore, kind: ProposalKind, artifact_id: str -) -> None: +) -> str | None: getter = getattr(store, _ARTIFACT_GETTERS[kind]) try: getter(artifact_id) except ArtifactNotFoundError: - return - raise ProposalError( + return None + return ( f"cannot approve: {kind.value} {artifact_id} already exists " f"(a prior approve may have been interrupted; reconcile manually " f"by removing the artifact or rejecting this proposal)" ) +def _ensure_no_existing_artifact( + store: KBStore, kind: ProposalKind, artifact_id: str +) -> None: + reason = _existing_artifact_block_reason(store, kind, artifact_id) + if reason: + raise ProposalError(reason) + + def _node_exists(store: KBStore, node_id: str) -> bool: """True if `node_id` resolves to a Claim, Page, Entity, or Source. diff --git a/tests/test_storage.py b/tests/test_storage.py index ce775688..6f6f3390 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -615,6 +615,30 @@ def test_check_approvable_clean_for_well_formed_proposal( assert check_approvable(store, pr.id, approved_by="reviewer") is None +def test_check_approvable_catches_id_collision_with_existing_claim( + store: KBStore, +) -> None: + """approve() refuses to overwrite an existing artifact; the batch + precheck must catch that too, or `vouch approve a b` can say "all + clear" and then partially apply before failing on the collision.""" + src = store.put_source(b"e") + first = propose_claim( + store, text="first", evidence=[src.id], + slug_hint="foo-bar", proposed_by="agent", + ) + approve(store, first.id, approved_by="reviewer") + + second = propose_claim( + store, text="second", evidence=[src.id], + slug_hint="foo-bar", proposed_by="agent", + ) + reason = check_approvable(store, second.id, approved_by="reviewer") + assert reason is not None + assert "already exists" in reason + with pytest.raises(ProposalError, match="already exists"): + approve(store, second.id, approved_by="reviewer") + + # --- evidence -------------------------------------------------------------