fix: remove duplicate session-manager setup logic across bot platform adapters#2135
fix: remove duplicate session-manager setup logic across bot platform adapters#2135praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
… 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>
|
@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 repeated session manager setup logic into a single
Confidence Score: 5/5Safe to merge — pure consolidation of well-tested per-adapter logic into a single helper with no changes to the runtime path for previously full-featured adapters. Every adapter that previously ran the full session-setup block (Discord, Slack, Telegram, WhatsApp) receives identical behavior from the helper. The three adapters that previously used a stripped-down constructor (email, linear, agentmail) now correctly honour max_history and session.reset, which is a net improvement. The store variable is set in all exception paths, SessionResetPolicy is already in scope via the module-level import, and run_control is threaded through correctly for Telegram. No logic is lost or incorrectly transformed. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Bot Adapter __init__] --> B[build_session_manager
config, platform, run_control]
B --> C{Import
get_default_session_store}
C -- ImportError --> D[store = None]
C -- success --> E{Call
get_default_session_store}
E -- Exception --> F[log warning
store = None]
E -- success --> G[store = result]
D & F & G --> H{config.session
.reset set?}
H -- yes --> I[SessionResetPolicy
.from_dict]
H -- no --> J[reset_policy = None]
I & J --> K{Resolve
max_history}
K -- config.max_history --> L[use config.max_history]
K -- session.max_history --> M[use session.max_history]
K -- neither --> N[default = 100]
L & M & N --> O[BotSessionManager
max_history, store, platform
reset_policy, run_control]
O --> P[self._session / self._session_mgr]
%%{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"}}}%%
flowchart TD
A[Bot Adapter __init__] --> B[build_session_manager
config, platform, run_control]
B --> C{Import
get_default_session_store}
C -- ImportError --> D[store = None]
C -- success --> E{Call
get_default_session_store}
E -- Exception --> F[log warning
store = None]
E -- success --> G[store = result]
D & F & G --> H{config.session
.reset set?}
H -- yes --> I[SessionResetPolicy
.from_dict]
H -- no --> J[reset_policy = None]
I & J --> K{Resolve
max_history}
K -- config.max_history --> L[use config.max_history]
K -- session.max_history --> M[use session.max_history]
K -- neither --> N[default = 100]
L & M & N --> O[BotSessionManager
max_history, store, platform
reset_policy, run_control]
O --> P[self._session / self._session_mgr]
Reviews (2): 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. |
- 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>
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