fix: add existence guards to all put_* methods (#12) - #26
Conversation
vouchdev#12) put_claim, put_page, put_entity, and put_relation wrote files unconditionally. When two artifacts shared the same slug, the second approval silently overwrote the first — data loss with no error or warning. Add an existence check before write in all four put_* methods, raising ValueError on collision. Closes vouchdev#12
📝 WalkthroughWalkthroughKBStore create methods now use exclusive file creation; if a target claim, page, entity, relation, evidence, session, or proposal file already exists, the methods raise ValueError instead of overwriting the existing file. ChangesDuplicate ID Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/storage.py`:
- Around line 276-279: The ValueError messages that reference "use update" are
misleading because there are no update_* APIs; locate the checks that call
self._page_path(page.id).exists() (and the analogous checks for entities and
relations in the same module, e.g., in create_page, create_entity,
create_relation) and change their exception text to only advise choosing a
different slug (for example: "page {page.id} already exists — choose a different
slug") so callers aren’t directed to nonexistent update APIs; update all three
occurrences noted in the review.
- Around line 237-240: The current check-then-write using
self._claim_path(...).exists() is race-prone; replace the existence check +
subsequent write with an atomic exclusive create using path.open("x") (or
Path.open with mode "x") and write the claim content, catching FileExistsError
and mapping it to the same ValueError("claim {claim.id} already exists — use
update_claim()"). Apply this change wherever self._claim_path(...) existence is
checked before writing (the shown block and the similar blocks around the other
occurrences referenced) so creation uses exclusive-create semantics and avoids
TOCTOU races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…hods
Use path.open('x') instead of exists()+write_text to avoid TOCTOU races.
Also fix error messages referencing nonexistent update_* APIs for page/entity/relation.
Closes vouchdev#12
|
Nice use of open("x") for exclusive create — it's inherently race-safe, which is better than a separate exists() check. Clean approach. A few things I noticed:
Overall |
- Replace bare write_text() with exclusive open("x") in all three methods
to prevent silent overwrites, matching the pattern from put_claim et al.
- Restore explanatory comment in put_claim about Source/Evidence ID duality.
- Normalize em dashes to ASCII "--" in error strings for terminal safety.
Addresses reviewer feedback on PR vouchdev#26.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vouch/storage.py (1)
246-252: ⚡ Quick winSpecify encoding explicitly and consider cleaner exception chaining.
All exclusive-create blocks open files in text mode without specifying
encoding. This uses the platform-dependent default encoding, which may not be UTF-8 on all systems. YAML files should be written as UTF-8 for portability.Additionally, re-raising
ValueErrorwithoutfrom Noneincludes theFileExistsErrortraceback, which may be confusing since theFileExistsErroris an implementation detail.♻️ Suggested improvements
For
put_claim(lines 246-252):try: - with self._claim_path(claim.id).open("x") as f: + with self._claim_path(claim.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(claim.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"claim {claim.id} already exists -- use update_claim()" - ) + ) from NoneFor
put_page(lines 282-288):try: - with self._page_path(page.id).open("x") as f: + with self._page_path(page.id).open("x", encoding="utf-8") as f: f.write(_serialize_page(page)) except FileExistsError: raise ValueError( f"page {page.id} already exists -- choose a different slug" - ) + ) from NoneFor
put_entity(lines 306-312):try: - with self._entity_path(entity.id).open("x") as f: + with self._entity_path(entity.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(entity.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"entity {entity.id} already exists -- choose a different slug" - ) + ) from NoneFor
put_relation(lines 331-337):try: - with self._relation_path(rel.id).open("x") as f: + with self._relation_path(rel.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(rel.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"relation {rel.id} already exists -- choose a different slug" - ) + ) from NoneFor
put_evidence(lines 364-370):try: - with self._evidence_path(ev.id).open("x") as f: + with self._evidence_path(ev.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(ev.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"evidence {ev.id} already exists -- choose a different slug" - ) + ) from NoneFor
put_session(lines 389-395):try: - with self._session_path(sess.id).open("x") as f: + with self._session_path(sess.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(sess.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"session {sess.id} already exists -- choose a different id" - ) + ) from NoneFor
put_proposal(lines 414-420):try: - with self._proposal_path(proposal.id).open("x") as f: + with self._proposal_path(proposal.id).open("x", encoding="utf-8") as f: f.write(_yaml_dump(proposal.model_dump(mode="json"))) except FileExistsError: raise ValueError( f"proposal {proposal.id} already exists -- choose a different id" - ) + ) from NoneAlso applies to: 282-288, 306-312, 331-337, 364-370, 389-395, 414-420
🤖 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/storage.py` around lines 246 - 252, The file-open calls in the exclusive-create blocks (e.g., put_claim using self._claim_path(...).open("x")) should explicitly set encoding="utf-8" to ensure YAML is written portably, and when converting the FileExistsError into a ValueError, re-raise with "from None" to suppress the underlying traceback; apply the same two changes to the other similar functions: put_page, put_entity, put_relation, put_evidence, put_session, and put_proposal so each open(..., "x", encoding="utf-8") and each raise ValueError(... ) uses "from None".
🤖 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.
Nitpick comments:
In `@src/vouch/storage.py`:
- Around line 246-252: The file-open calls in the exclusive-create blocks (e.g.,
put_claim using self._claim_path(...).open("x")) should explicitly set
encoding="utf-8" to ensure YAML is written portably, and when converting the
FileExistsError into a ValueError, re-raise with "from None" to suppress the
underlying traceback; apply the same two changes to the other similar functions:
put_page, put_entity, put_relation, put_evidence, put_session, and put_proposal
so each open(..., "x", encoding="utf-8") and each raise ValueError(... ) uses
"from None".
|
LGTM! |
fix: add existence guards to all put_* methods (#12)
Summary
put_claim,put_page,put_entity, andput_relationwrote files unconditionally — no existence check. When two artifacts shared the same slug, the second approval silently overwrote the first. The original artifact was lost with no error or warning.Fix
Add an existence check before write in all four
put_*methods, raisingValueErroron collision so callers must explicitly useupdate_*or choose a different slug.Related Issues
Closes #12
Summary by CodeRabbit