[bug] crystallize crashes on retry after partial approval

[glorysr1209-png]

Summary

Closes #139.

Upsert the session summary page on crystallize retry and rebuild claims from all approved session proposals.

Changes

• Collect approved claim IDs across the session.
• Overwrite existing summary page file + reindex when the page already exists.
• Add test_crystallize_retry_after_partial_approval_updates_summary_page.

Test plan

• pytest tests/test_sessions.py -k crystallize_retry

Summary by CodeRabbit

• Bug Fixes
  • Fixed session summary page generation to correctly reflect approved claims instead of artifacts.
  • Enhanced session crystallization to recover from transient failures during the approval process and update summary pages with all recovered claims.

[coderabbitai]

Warning

Review limit reached

@glorysr1209-png, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 59 minutes and 35 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d3675aff-e052-49c3-a86a-7897b6d49de8

📥 Commits

Reviewing files that changed from the base of the PR and between 94b002e and 8c8bb9d.

📒 Files selected for processing (2)

• src/vouch/sessions.py
• src/vouch/storage.py

📝 Walkthrough

Walkthrough

The PR fixes a crash in session crystallization when retrying after partial approval. It extracts approved claim IDs from proposals using a new helper, derives the summary page from those claims instead of artifact IDs, and implements page upsert logic to conditionally overwrite or create the page. A new test validates retry behavior after transient approval failure.

Changes

Session Crystallize Summary Page Upsert

Layer / File(s)	Summary
Claim extraction and imports src/vouch/sessions.py	Updated imports add ProposalKind for proposal filtering and _serialize_page for page serialization. New _approved_claim_ids_for_session() filters approved proposals by session and kind, extracts deduped claim IDs from payload, and returns sorted list.
Summary page upsert in crystallize src/vouch/sessions.py	Crystallize now uses the claim ID helper instead of approved artifact IDs. Summary page writing conditionally overwrites an existing page file (serialize and embed) or creates new page via put_page when absent, then indexes and sets summary_page_id.
Partial approval retry test tests/test_sessions.py	Test verifies crystallize recovers from transient approval failure: first run approves one claim and produces summary page, second run approves remaining claim, and the captured summary page contains both claims after retry.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• [bug] crystallize crashes on retry after partial approval #139: Directly addresses the bug where crystallize crashes on page already exists during retry after partial approval by implementing page upsert behavior.

Possibly related PRs

• vouchdev/vouch#77: Both PRs modify the crystallize flow for building/upserting the session summary Page and wiring the resulting summary_page_id.
• vouchdev/vouch#61: Both PRs modify crystallize around writing the session-summary Page with updates, embedding, and FTS5 indexing.

Poem

🐰 A session crystallizes with grace,
No crashes when retried in place!
The summary page now bends, doesn't break,
Upsert and recover for goodness sake.
hops happily ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title accurately and concisely describes the main bug fix: preventing crystallize from crashing on retry after partial approval.
Linked Issues check	✅ Passed	Changes fully address issue #139 by implementing upsert behavior for summary pages and collecting all approved claims across retries.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the crystallize retry crash and implementing upsert behavior for summary pages.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/vouch/sessions.py (1)

106-116: ⚡ Quick win

Reduce coupling to storage internals in the upsert path.

This block re-implements KBStore.put_page's persistence (serialize + _embed_and_store) by reaching into private members, and independently hardcodes the on-disk claim path on Line 108. Two concerns:

• The hardcoded claims/{cid}.yaml duplicates store._claim_path(cid) used by put_page. If that layout ever changes, the existence check silently filters all claims out, producing a summary page with an empty claims list rather than failing — a silent data-quality regression.
• The overwrite branch duplicates put_page's serialize+embed flow, so future changes there (e.g. added validation) won't apply on retry. A KBStore.upsert_page(page) (or put_page(page, overwrite=True)) would keep create/update paths in lockstep. As a side note, constructing a fresh Page on each overwrite also resets created_at; an upsert method could preserve it.

♻️ Minimal in-file fix for the claim-path duplication

             claims=[
-                cid for cid in session_claim_ids
-                if (store.kb_dir / "claims" / f"{cid}.yaml").exists()
+                cid for cid in session_claim_ids
+                if store._claim_path(cid).exists()
             ],

Encapsulating the overwrite-vs-create branch behind a KBStore method would require a change in src/vouch/storage.py.

🤖 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 106 - 116, The upsert flow in sessions.py
duplicates KBStore internals and hardcodes claim paths; replace the manual claim
existence check and serialize/embed logic with a single KBStore method to avoid
drift: add a new KBStore.upsert_page(page, overwrite=True) (or extend
KBStore.put_page to accept overwrite flag) that internally uses
store._claim_path(...) and preserves created_at, performs serialization and
calls store._embed_and_store(...) as needed, then call that new method from
sessions.py instead of inspecting store._page_path, writing the file, or
checking claims directly.

🤖 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/sessions.py`:
- Around line 106-116: The upsert flow in sessions.py duplicates KBStore
internals and hardcodes claim paths; replace the manual claim existence check
and serialize/embed logic with a single KBStore method to avoid drift: add a new
KBStore.upsert_page(page, overwrite=True) (or extend KBStore.put_page to accept
overwrite flag) that internally uses store._claim_path(...) and preserves
created_at, performs serialization and calls store._embed_and_store(...) as
needed, then call that new method from sessions.py instead of inspecting
store._page_path, writing the file, or checking claims directly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 66b4e057-2177-4f8e-ba11-04d73710dccf

📥 Commits

Reviewing files that changed from the base of the PR and between 3beb821 and 94b002e.

📒 Files selected for processing (2)

• src/vouch/sessions.py
• tests/test_sessions.py

[plind-junior]

the _approved_claim_ids_for_session helper is exactly the right lever — rebuilding the claim list from the full approved-proposals history (not just the current run's batch) is what makes the summary page accurate on retry, and the upsert_page path in storage.py keeps the storage contract clean.

one thing worth double-checking: upsert_page writes a fresh Page(...) object each time, which gets a brand-new created_at (the field has default_factory=utcnow). on a first-run-succeeded flow that's fine, but on a retry the summary page quietly gets a newer created_at than the claims it links to. could upsert_page accept the original page's created_at — or could sessions.py read the existing page first and reuse its timestamp before overwriting? small correctness nit, but audit tooling that sorts by created_at would see the session page appear after its own claims.

non-blocking: _approved_claim_ids_for_session calls store.list_proposals(ProposalStatus.APPROVED) which scans both proposed/ and decided/ subdirs and deserialises every YAML on each crystallize call. for a large KB this is the slow path. not urgent, but worth a # TODO: add session_id index to decided/ dir comment so it's visible when the perf bar matters.

small process note: no CHANGELOG.md entry — the PR checklist probably already flags this, but just surfacing it.

thanks for tracking down the retry crash end-to-end and writing the flaky-approve test to nail it 🙏