fix: remove duplicate session-manager setup logic across bot platform adapters#2135
Conversation
|
@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.
|
|
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:
📝 WalkthroughWalkthroughA new ChangesSession Manager Factory and Adapter Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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)
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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR consolidates the duplicated session manager initialization logic (store acquisition, reset policy extraction, and
Confidence Score: 5/5Safe to merge — the refactor is mechanically correct, all code paths in the helper assign The change is a pure consolidation of identical code blocks; the helper correctly covers every branch (ImportError, runtime exception, and success) and the
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant A as Bot Adapter
participant H as build_session_manager()
participant S as get_default_session_store()
participant M as BotSessionManager
A->>H: build_session_manager(config, platform, run_control?)
H->>S: get_default_session_store()
alt ImportError
H-->>H: "store = None"
else runtime exception
S-->>H: Exception
H-->>H: "store = None"
else success
S-->>H: store
end
H->>H: extract reset_policy from config.session.reset
H->>H: resolve max_history
H->>M: BotSessionManager(max_history, store, platform, reset_policy, run_control)
M-->>H: instance
H-->>A: BotSessionManager instance
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant A as Bot Adapter
participant H as build_session_manager()
participant S as get_default_session_store()
participant M as BotSessionManager
A->>H: build_session_manager(config, platform, run_control?)
H->>S: get_default_session_store()
alt ImportError
H-->>H: "store = None"
else runtime exception
S-->>H: Exception
H-->>H: "store = None"
else success
S-->>H: store
end
H->>H: extract reset_policy from config.session.reset
H->>H: resolve max_history
H->>M: BotSessionManager(max_history, store, platform, reset_policy, run_control)
M-->>H: instance
H-->>A: BotSessionManager instance
Reviews (3): Last reviewed commit: "fix: improve session store error handlin..." | Re-trigger Greptile |
![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/bots/_session.py`:
- Around line 821-825: The try-except block in the get_default_session_store
initialization silently catches all exceptions without logging, masking real
configuration or runtime failures. Modify the exception handling to distinguish
between ImportError (which should silently fall back to in-memory store) and
other exceptions (which should be logged as warnings or errors with the
exception details) so operators can detect misconfiguration or outages in the
session store initialization.
🪄 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: 325f90e6-48d5-47fd-a6e3-d76bfb530ac4
📒 Files selected for processing (8)
src/praisonai/praisonai/bots/_session.pysrc/praisonai/praisonai/bots/agentmail.pysrc/praisonai/praisonai/bots/discord.pysrc/praisonai/praisonai/bots/email.pysrc/praisonai/praisonai/bots/linear.pysrc/praisonai/praisonai/bots/slack.pysrc/praisonai/praisonai/bots/telegram.pysrc/praisonai/praisonai/bots/whatsapp.py
| try: | ||
| from praisonaiagents.session import get_default_session_store | ||
| store = get_default_session_store() | ||
| except Exception: | ||
| store = None |
There was a problem hiding this comment.
Avoid silent fallback when default session-store initialization fails.
Line 824 currently swallows all store-init failures and silently switches to in-memory sessions, which can cause unnoticed loss of persisted history across all adapters. Keep the fallback, but log non-import failures so operators can detect misconfiguration/outages.
Proposed fix
- try:
- from praisonaiagents.session import get_default_session_store
- store = get_default_session_store()
- except Exception:
- store = None
+ try:
+ from praisonaiagents.session import get_default_session_store
+ except ImportError:
+ store = None
+ else:
+ try:
+ store = get_default_session_store()
+ except Exception as exc:
+ logger.warning(
+ "Default session store unavailable; falling back to in-memory store: %s",
+ exc,
+ )
+ store = None🧰 Tools
🪛 Ruff (0.15.17)
[warning] 824-824: Do not catch blind exception: Exception
(BLE001)
🤖 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/bots/_session.py` around lines 821 - 825, The
try-except block in the get_default_session_store initialization silently
catches all exceptions without logging, masking real configuration or runtime
failures. Modify the exception handling to distinguish between ImportError
(which should silently fall back to in-memory store) and other exceptions (which
should be logged as warnings or errors with the exception details) so operators
can detect misconfiguration or outages in the session store initialization.
Source: Linters/SAST tools
![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 |
|
Claude finished @MervinPraison's task —— View job ✅ Architecture Review CompleteTodo List:
Review SummaryArchitecture Validation ✅
Issues Fixed
What I Skipped
Final Verdict✅ APPROVED - This is a well-executed refactor that:
The PR is ready to merge. |
|
Automated merge readiness report (local git rebase origin/main scan, 2026-06-22 22:04 UTC) GitHub status
CI (key jobs: test-core, smoke, test-windows, main (3.11), review)
Conflicts (git rebase origin/main)Conflict files (1):
|
|
Claude finished @MervinPraison's task —— View job ✅ Merge Conflicts ResolvedTodo List:
Summary of ChangesConflict Resolution:
Files Modified:
Validation:
Final Verdict: MERGE-READY 🎉The PR is now rebased on latest main with all conflicts resolved. CI jobs were already green before the rebase, and the changes are minimal (only combining two non-conflicting additions). |
… adapters (fixes #2134) - Created build_session_manager helper function in _session.py - Updated all 7 bot adapters to use the new helper - Removed ~15-20 lines of duplicate code per adapter - Maintains identical runtime behavior with no API changes Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
- Distinguish between ImportError (silent fallback) and other exceptions (log warning) - Remove unused BotSessionManager imports from agentmail.py and linear.py - Addresses CodeRabbit and Greptile review feedback Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-2134-20260621-1608
branch
from
83e1e61 to
4eb8116
Compare
… adapters (#2135) * fix: remove duplicate session-manager setup logic across bot platform adapters (fixes #2134) - Created build_session_manager helper function in _session.py - Updated all 7 bot adapters to use the new helper - Removed ~15-20 lines of duplicate code per adapter - Maintains identical runtime behavior with no API changes Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com> * fix: improve session store error handling and remove unused imports - Distinguish between ImportError (silent fallback) and other exceptions (log warning) - Remove unused BotSessionManager imports from agentmail.py and linear.py - Addresses CodeRabbit and Greptile review feedback Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com> --------- Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #2134
This PR removes duplicate session manager setup logic that was copied across 7+ bot platform adapters by extracting it into a shared helper function.
Changes:
Benefits:
This is a pure refactor with no behavioral changes.
Summary by CodeRabbit