fix: dedupe conversation-persistence session flow into one owner#3170
fix: dedupe conversation-persistence session flow into one owner#3170praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
#3167) Deduplicate the create-or-resume session logic that PraisonAIDB and PersistenceOrchestrator each reimplemented across their sync and async surfaces. A new internal helper persistence/conversation/_ops.py owns the store-level flow; both classes call it while keeping their distinct return-type contracts (DbMessage vs ConversationMessage), caching, and lock/cooldown machinery. Behaviour-preserving. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSession resume-or-create logic is centralized in synchronous and asynchronous conversation helpers. The database adapter and persistence orchestrator now use these helpers while preserving their existing session caching, message conversion, and sync/async store dispatch behavior. ChangesConversation persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR centralizes conversation session creation and resume behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: build orchestrator session lazily s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3170 ✅Todo
Phase 1 — Architecture (AGENTS.md)
Phase 2 — Valid issue fixedGreptile (P2) — resume path constructed a discarded Fix ( created: List[ConversationSession] = []
def _build_session() -> ConversationSession:
agent_id = getattr(agent, "name", None) or getattr(agent, "agent_id", None)
new_session = ConversationSession(..., agent_id=agent_id, ...)
created.append(new_session)
return new_session
messages = resume_or_create_session(..., build_session=_build_session, ...)
if messages is None:
new_session = created[0] # exact instance persisted → cache identity preservedNote: Independent review: Phase 3 — Tests
Verdict: Approve ✅The refactor cleanly de-duplicates the store-level session flow, preserves all contracts, and the one valid reviewer concern (eager resume-path construction) is now fixed. Pushed to Files modified this review: |
![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
🤖 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/praisonai/praisonai/persistence/conversation/_ops.py`:
- Around line 20-47: The synchronous session-resume helper must use a
caller-provided creation callback so async stores are dispatched through the
orchestrator bridge. In
src/praisonai/praisonai/persistence/conversation/_ops.py#L20-L47, add a
create_session callback parameter and invoke it when session is absent instead
of calling store.create_session directly; in
src/praisonai/praisonai/persistence/orchestrator.py#L191-L203, pass a callback
that routes self.conversation.create_session through self._sync before caching
the session.
🪄 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
Run ID: a4a6ee3e-8a2d-420c-8120-c9ab88e94d9f
📒 Files selected for processing (3)
src/praisonai/praisonai/db/adapter.pysrc/praisonai/praisonai/persistence/conversation/_ops.pysrc/praisonai/praisonai/persistence/orchestrator.py
| def resume_or_create_session( | ||
| store: Any, | ||
| session: Optional[ConversationSession], | ||
| session_id: str, | ||
| build_session: Callable[[], ConversationSession], | ||
| get_messages: Callable[[], List[ConversationMessage]], | ||
| ) -> Optional[List[ConversationMessage]]: | ||
| """Create the session if missing, else return its messages (sync). | ||
|
|
||
| Args: | ||
| store: The conversation store. | ||
| session: Result of the caller's ``get_session`` lookup (``None`` when | ||
| the session does not yet exist). | ||
| session_id: The session identifier (unused directly; kept for parity | ||
| and readability at call sites). | ||
| build_session: Factory returning the ``ConversationSession`` to create | ||
| when ``session`` is ``None`` — lets each caller keep its own name/ | ||
| metadata conventions. | ||
| get_messages: Callable returning the existing messages when resuming. | ||
|
|
||
| Returns: | ||
| ``None`` when a new session was created (no history), otherwise the list | ||
| of previously persisted messages. | ||
| """ | ||
| if session is None: | ||
| store.create_session(build_session()) | ||
| return None | ||
| return get_messages() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Preserve async-store dispatch in the synchronous creation path.
The sync helper directly calls create_session, bypassing the orchestrator’s coroutine bridge.
src/praisonai/praisonai/persistence/conversation/_ops.py#L20-L47: accept and invoke a caller-providedcreate_sessioncallback.src/praisonai/praisonai/persistence/orchestrator.py#L191-L203: passlambda s: self._sync(self.conversation.create_session(s))before caching the session.
📍 Affects 2 files
src/praisonai/praisonai/persistence/conversation/_ops.py#L20-L47(this comment)src/praisonai/praisonai/persistence/orchestrator.py#L191-L203
🤖 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/praisonai/praisonai/persistence/conversation/_ops.py` around lines 20 -
47, The synchronous session-resume helper must use a caller-provided creation
callback so async stores are dispatched through the orchestrator bridge. In
src/praisonai/praisonai/persistence/conversation/_ops.py#L20-L47, add a
create_session callback parameter and invoke it when session is absent instead
of calling store.create_session directly; in
src/praisonai/praisonai/persistence/orchestrator.py#L191-L203, pass a callback
that routes self.conversation.create_session through self._sync before caching
the session.
…gent identity Addresses Greptile review: on_agent_start/aon_agent_start constructed a ConversationSession eagerly on every call, reading agent.name/agent.agent_id and building an object discarded on resume. Move construction into the build_session factory (create-branch only) while capturing the created instance for identical cache identity/timestamps. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3167
Summary
PraisonAIDB(db/adapter.py) andPersistenceOrchestrator(persistence/orchestrator.py) each reimplemented the same create-or-resume session flow over the samepersistence.conversationstore, across both their sync and async surfaces. This duplication had already begun to drift. This PR gives that store-level flow a single owner.Changes
persistence/conversation/_ops.pywithresume_or_create_session(sync) andaresume_or_create_session(async): given the caller'sget_sessionresult, it either creates the session (via a caller-supplied factory) or returns the existing messages.PraisonAIDB.on_agent_start/aon_agent_startnow call the helper, keeping theirDbMessagereturn contract and_dispatch_asyncdiscipline.PersistenceOrchestrator.on_agent_start/aon_agent_startnow call the helper, keeping theirConversationMessagereturn contract, session caching,resumeflag, and sync/async off-loop dispatch.Behaviour preserved
DbMessagevsConversationMessage) unchanged.build_sessionfactory.ConversationSessioninstance it persists (same timestamps/identity).db/adapter.pyuntouched.Testing
test_db_adapter_*,test_turso_store,test_serverless_postgres).resume=Falsepaths for both classes (sync + async) — all pass.pydantic), unrelated to this change.Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor