fix(proposals): refuse to overwrite existing artifact on approve#27
Merged
plind-junior merged 1 commit intoMay 19, 2026
Merged
Conversation
approve() wrote the durable artifact (put_claim, put_page, put_entity, put_relation) before transitioning the proposal to APPROVED via move_proposal_to_decided. A crash between those steps would leave the proposal PENDING with the artifact on disk, and a retry would silently rewrite the artifact with new approved_by / created_at metadata. Guard the artifact write with an existence check; raise ProposalError when an artifact at the target id already exists so the partial state is visible and the human can reconcile (remove the artifact or reject the proposal). Fixes vouchdev#11
📝 WalkthroughWalkthroughThe PR adds an idempotent guard to ChangesOverwrite Prevention in Proposal Approval
Sequence DiagramsequenceDiagram
participant Caller
participant approve
participant _ensure_no_existing_artifact
participant StoreGetter
participant StorageWrite
Caller->>approve: approve(proposal)
approve->>_ensure_no_existing_artifact: check artifact existence
_ensure_no_existing_artifact->>StoreGetter: get_claim/page/entity/relation
StoreGetter-->>_ensure_no_existing_artifact: ArtifactNotFoundError or artifact
alt artifact found
_ensure_no_existing_artifact-->>approve: raise ProposalError
approve-->>Caller: ProposalError
else artifact not found
_ensure_no_existing_artifact-->>approve: proceed
approve->>StorageWrite: put_claim/page/entity/relation
StorageWrite-->>approve: success
approve->>approve: move_proposal_to_decided
approve-->>Caller: approval complete
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_storage.py (1)
225-245: ⚡ Quick winCover the other artifact kinds with this regression too.
This only exercises the claim path, but the new guard added branch-specific getter dispatch for pages, entities, and relations as well. A small parametrized variant here would lock in the full
ProposalKind→getter contract and catch mapping regressions early.🤖 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 `@tests/test_storage.py` around lines 225 - 245, Extend test_approve_refuses_to_overwrite_existing_artifact to parametrize over ProposalKind variants (claim, page, entity, relation) and exercise each artifact getter/putter instead of only get_claim; for each kind create a pre-existing artifact via the appropriate store.put_<kind> (matching how propose_<kind> / approve() expect), create a proposal with the same slug via propose_claim/propose_page/etc. (or by converting propose_claim to a generic propose helper), then call approve(store, pr.id, approved_by=...) inside pytest.raises(... "already exists") and assert the original artifact remained unchanged and proposal.status is ProposalStatus.PENDING; implement a small mapping in the test from ProposalKind to the store methods and any propose helper so the same assertions used with get_claim and get_proposal are applied for pages, entities, and relations as well.
🤖 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 `@src/vouch/proposals.py`:
- Around line 311-323: The current TOCTOU guard in
_ensure_no_existing_artifact(…) is racy because it does a separate read before
the write; instead, modify the KBStore write API (e.g., methods referenced by
_ARTIFACT_GETTERS such as put_claim, put_page, put_<other>) to support an atomic
"create-only" operation (or a fail-if-exists flag) that fails with a distinct
exception (e.g., ArtifactAlreadyExistsError) when the artifact exists, and
update the approve() code path to call those create-only writes directly (or
call the existing write and handle the new exception) rather than performing the
preflight get; finally, remove or stop relying on _ensure_no_existing_artifact
and convert its failure handling to catch the storage-level conflict and raise
ProposalError on that storage-level exception so the write is atomic and cannot
be overwritten by concurrent approves.
---
Nitpick comments:
In `@tests/test_storage.py`:
- Around line 225-245: Extend
test_approve_refuses_to_overwrite_existing_artifact to parametrize over
ProposalKind variants (claim, page, entity, relation) and exercise each artifact
getter/putter instead of only get_claim; for each kind create a pre-existing
artifact via the appropriate store.put_<kind> (matching how propose_<kind> /
approve() expect), create a proposal with the same slug via
propose_claim/propose_page/etc. (or by converting propose_claim to a generic
propose helper), then call approve(store, pr.id, approved_by=...) inside
pytest.raises(... "already exists") and assert the original artifact remained
unchanged and proposal.status is ProposalStatus.PENDING; implement a small
mapping in the test from ProposalKind to the store methods and any propose
helper so the same assertions used with get_claim and get_proposal are applied
for pages, entities, and relations as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3765562b-9fb8-49ff-8775-99961b7767d2
📒 Files selected for processing (2)
src/vouch/proposals.pytests/test_storage.py
Comment on lines
+311
to
+323
| def _ensure_no_existing_artifact( | ||
| store: KBStore, kind: ProposalKind, artifact_id: str | ||
| ) -> None: | ||
| getter = getattr(store, _ARTIFACT_GETTERS[kind]) | ||
| try: | ||
| getter(artifact_id) | ||
| except ArtifactNotFoundError: | ||
| return | ||
| raise ProposalError( | ||
| 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)" | ||
| ) |
There was a problem hiding this comment.
Make the existence guard atomic with the write.
This is still a TOCTOU check: two approve() calls can both observe “missing” here and then both reach put_claim()/put_page()/…; the second write can still overwrite the first artifact. The overwrite refusal needs to live in the storage write path itself (exclusive create or per-artifact locking), not as a separate preflight read.
🤖 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/proposals.py` around lines 311 - 323, The current TOCTOU guard in
_ensure_no_existing_artifact(…) is racy because it does a separate read before
the write; instead, modify the KBStore write API (e.g., methods referenced by
_ARTIFACT_GETTERS such as put_claim, put_page, put_<other>) to support an atomic
"create-only" operation (or a fail-if-exists flag) that fails with a distinct
exception (e.g., ArtifactAlreadyExistsError) when the artifact exists, and
update the approve() code path to call those create-only writes directly (or
call the existing write and handle the new exception) rather than performing the
preflight get; finally, remove or stop relying on _ensure_no_existing_artifact
and convert its failure handling to catch the storage-level conflict and raise
ProposalError on that storage-level exception so the write is atomic and cannot
be overwritten by concurrent approves.
Member
|
LGTM! |
This was referenced May 21, 2026
plind-junior
added a commit
that referenced
this pull request
May 22, 2026
…act-guard fix(proposals): refuse to overwrite existing artifact on approve
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
approve()now refuses to overwrite an artifact that already exists at the target id, surfacing partially-applied state instead of silently rewriting it.Root Cause
src/vouch/proposals.py:213-269:
approve()writes the durable artifact (put_claim/put_page/put_entity/put_relation) before transitioning the proposal toAPPROVEDviamove_proposal_to_decided. If the process crashes between the two operations, the proposal staysPENDINGwhile the artifact is already on disk. A second approval call would then silently overwrite that artifact with newapproved_by/created_atmetadata.Fix
Before writing the artifact, check whether one already exists at
payload["id"]via the appropriateKBStore.get_*method. If it does, raiseProposalErrorso the human can reconcile manually (remove the stale artifact and re-approve, or reject the proposal). The check uses publicKBStoregetters that already raiseArtifactNotFoundErroron miss, so no new storage surface is introduced.The reorder-status-first alternative was rejected because it just moves the same failure window: a crash between status write and artifact write would leave a proposal marked
APPROVEDwith no artifact, which is harder to recover from than the current direction. Failing loud on conflict is the conservative choice and matches the existingproposal.status != PENDINGguard.Test Plan
tests/test_storage.py::test_approve_refuses_to_overwrite_existing_artifact— pre-seeds a claim at the target id, callsapprove(), assertsProposalError("already exists")is raised and the pre-existing claim is untouched.DID NOT RAISE) and passes with the guard in place.pytest tests/test_storage.py -k "approve or propose"pytest -q→55 passedruff check src tests→All checks passed!mypy src→ unchanged (one pre-existing error incontext.pyunrelated to this PR)Closes #11
Summary by CodeRabbit
Bug Fixes
Tests