fix(sessions): make crystallize retry idempotent for summary pages - #256
Merged
plind-junior merged 1 commit intoJul 6, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Changescrystallize() retry upsert fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/sessions.py`:
- Around line 110-115: The current check-then-act pattern in the store.get_page,
store.put_page, and store.update_page sequence creates a race condition where
concurrent calls can both pass the get_page check and then fail when put_page
tries to create an already-existing page. Reverse the logic to attempt put_page
first and catch the "already exists" error (likely a specific exception), then
fall back to store.update_page in the exception handler instead of the
try-except-else structure. This makes the upsert operation race-safe by handling
the concurrent creation case gracefully.
In `@src/vouch/storage.py`:
- Around line 353-358: The update_page method is missing claim existence
validation that is enforced in put_page(), allowing pages to be persisted with
dangling claims references. Add the same claim validation logic used in
put_page() to the update_page method before the page is written to storage
(before the _page_path write_text call). This ensures that all claims referenced
in the page being updated are verified to exist, maintaining page-claim
integrity.
🪄 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: f0af788b-afb8-4c55-ab8e-4f8457a86a0b
📒 Files selected for processing (4)
CHANGELOG.mdsrc/vouch/sessions.pysrc/vouch/storage.pytests/test_sessions.py
Comment on lines
+110
to
+115
| try: | ||
| store.get_page(page.id) | ||
| except ArtifactNotFoundError: | ||
| store.put_page(page) | ||
| else: | ||
| store.update_page(page) |
There was a problem hiding this comment.
Make summary-page upsert race-safe.
Lines 110-115 use a check-then-create flow. Concurrent crystallize() calls can both miss on get_page(), then one put_page() fails with “already exists”, breaking retry safety.
Suggested patch
- try:
- store.get_page(page.id)
- except ArtifactNotFoundError:
- store.put_page(page)
- else:
- store.update_page(page)
+ try:
+ store.update_page(page)
+ except ArtifactNotFoundError:
+ try:
+ store.put_page(page)
+ except ValueError:
+ # Lost create race; page now exists.
+ store.update_page(page)🤖 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/sessions.py` around lines 110 - 115, The current check-then-act
pattern in the store.get_page, store.put_page, and store.update_page sequence
creates a race condition where concurrent calls can both pass the get_page check
and then fail when put_page tries to create an already-existing page. Reverse
the logic to attempt put_page first and catch the "already exists" error (likely
a specific exception), then fall back to store.update_page in the exception
handler instead of the try-except-else structure. This makes the upsert
operation race-safe by handling the concurrent creation case gracefully.
Comment on lines
+353
to
+358
| def update_page(self, page: Page) -> Page: | ||
| if not self._page_path(page.id).exists(): | ||
| raise ArtifactNotFoundError(f"page {page.id}") | ||
| self._page_path(page.id).write_text(_serialize_page(page)) | ||
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | ||
| return page |
There was a problem hiding this comment.
Preserve page-claim integrity checks in update_page.
Line 353 adds an update path that skips the claim existence validation enforced by put_page(). That allows persisting pages with dangling claims references.
Suggested patch
def update_page(self, page: Page) -> Page:
+ for cid in page.claims:
+ if not self._claim_path(cid).exists():
+ raise ValueError(f"page {page.id} references unknown claim {cid}")
if not self._page_path(page.id).exists():
raise ArtifactNotFoundError(f"page {page.id}")
self._page_path(page.id).write_text(_serialize_page(page))
self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}")
return page📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def update_page(self, page: Page) -> Page: | |
| if not self._page_path(page.id).exists(): | |
| raise ArtifactNotFoundError(f"page {page.id}") | |
| self._page_path(page.id).write_text(_serialize_page(page)) | |
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | |
| return page | |
| def update_page(self, page: Page) -> Page: | |
| for cid in page.claims: | |
| if not self._claim_path(cid).exists(): | |
| raise ValueError(f"page {page.id} references unknown claim {cid}") | |
| if not self._page_path(page.id).exists(): | |
| raise ArtifactNotFoundError(f"page {page.id}") | |
| self._page_path(page.id).write_text(_serialize_page(page)) | |
| self._embed_and_store(kind="page", id=page.id, text=f"{page.title}\n\n{page.body}") | |
| return page |
🤖 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 353 - 358, The update_page method is
missing claim existence validation that is enforced in put_page(), allowing
pages to be persisted with dangling claims references. Add the same claim
validation logic used in put_page() to the update_page method before the page is
written to storage (before the _page_path write_text call). This ensures that
all claims referenced in the page being updated are verified to exist,
maintaining page-claim integrity.
Member
|
Thanks for your contribution! Fix the conflict |
Contributor
Author
|
@plind-junior Thanks for the earlier note on the conflict — rebased onto latest Working through the CodeRabbit race/validation notes next; happy to push an update if you’d rather review the core retry fix first. Appreciate the review 🙏 |
Partial crystallize runs that already wrote session-{id} summary pages
failed on retry with "page already exists". Upsert the summary page
and derive its artifact list from all approved session proposals.
Fixes vouchdev#139
Co-authored-by: Cursor <cursoragent@cursor.com>
Yurii214
force-pushed
the
fix/139-crystallize-retry-idempotency
branch
from
June 29, 2026 02:37
8440927 to
0f9687e
Compare
dripsmvcp
added a commit
to dripsmvcp/vouch
that referenced
this pull request
Jul 13, 2026
five fixes to code merged on the test branch, all variants of "a failure or not-yet-ready state reported as success": - install-mcp (codex toml_merge): a config.toml the minimal serializer can't round-trip (non-bmp string value, nan/inf float) was bucketed as skipped and printed "(already present)" with a clean "Done", so the user believed vouch was wired into codex when it wasn't. add a distinct `failed` bucket, report it, and exit non-zero. - install-mcp: reject a manifest `dst` that escapes the target tree (`..`/absolute) instead of writing outside it — defense in depth for the manifest file writer. shipped adapters use only contained relative paths. - dual-solve review-ui: `_serialize` computed the recommendation over the empty candidate list on every poll, so a running job showed "neither engine produced a usable diff" for its whole duration. omit the hint until candidates exist. - session.crystallize: retrying on a not-yet-ended session rewrote the summary page with a fresh wall-clock "Ended:" stamp each time (and re-embedded), so vouchdev#256's "idempotent retry" wasn't. render a stable marker for an open session. - capture ingest-codex: cap rollout parsing at 64 MiB so an oversized or newline-free-blob rollout can't be read whole into memory, matching the byte caps on other untrusted reads. each fix has a regression test; make check green.
plind-junior
pushed a commit
that referenced
this pull request
Jul 13, 2026
…#402) five fixes to code merged on the test branch, all variants of "a failure or not-yet-ready state reported as success": - install-mcp (codex toml_merge): a config.toml the minimal serializer can't round-trip (non-bmp string value, nan/inf float) was bucketed as skipped and printed "(already present)" with a clean "Done", so the user believed vouch was wired into codex when it wasn't. add a distinct `failed` bucket, report it, and exit non-zero. - install-mcp: reject a manifest `dst` that escapes the target tree (`..`/absolute) instead of writing outside it — defense in depth for the manifest file writer. shipped adapters use only contained relative paths. - dual-solve review-ui: `_serialize` computed the recommendation over the empty candidate list on every poll, so a running job showed "neither engine produced a usable diff" for its whole duration. omit the hint until candidates exist. - session.crystallize: retrying on a not-yet-ended session rewrote the summary page with a fresh wall-clock "Ended:" stamp each time (and re-embedded), so #256's "idempotent retry" wasn't. render a stable marker for an open session. - capture ingest-codex: cap rollout parsing at 64 MiB so an oversized or newline-free-blob rollout can't be read whole into memory, matching the byte caps on other untrusted reads. each fix has a regression test; make check green.
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
3 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
crystallize()runs that already wrote asession-{id}summary page no longer fail on retry withpage session-... already exists.KBStore.update_page()and upserts the summary page (put_pageon first write,update_pageon retry).Why
Operators retry
crystallizeafter transient approval failures. The first partial run could approve one proposal and create the summary page; a second run then calledput_page()again and crashed before completing.What might break
pages/session-{id}.mdpath and shape.crystallizeon a session that already has approved proposals but no pending ones will now create or refresh the summary page whenwrite_summary_page=True.put_page()remains exclusive-create everywhere else.Tests
PYTHONPATH=src uv run pytest tests/test_sessions.pyPYTHONPATH=src uv run pytest --ignore=tests/embeddings(full suite minus optional embedding/MCP extras)PYTHONPATH=src uv run ruff check src/vouch/sessions.py src/vouch/storage.py tests/test_sessions.pytest_crystallize_retry_updates_existing_summary_page(partial approve → retry → same summary page lists all claims)Fixes #139
Summary by CodeRabbit
Bug Fixes
Tests