fix: Wrapper layer: global OpenAI client singleton, unsafe default bind, and duplicated ScheduleParser#2085
Conversation
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? |
📝 WalkthroughWalkthroughThree architecture fixes from issue ChangesArchitecture Gap Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces host configuration options for the call server, defaults the host to localhost with a warning for wildcard bindings, moves ScheduleParser to a shared module, and removes the global LRU cache for OpenAI clients in favor of per-call creation. Regarding the feedback, creating a new OpenAI client on every structured completion call is inefficient as it bypasses connection pooling; caching the client instance dynamically on the instance is recommended to avoid socket exhaustion and reduce latency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if _check_openai_available(): | ||
| client = _get_openai_client( | ||
| client = _create_openai_client( | ||
| api_key=self.config_list[0].get('api_key'), | ||
| base_url=self.config_list[0].get('base_url') | ||
| ) |
There was a problem hiding this comment.
Creating a new OpenAI client on every call to _structured_completion is inefficient because it bypasses connection pooling (managed internally by httpx.Client). This can lead to increased latency from repeated TCP/TLS handshakes and potential socket exhaustion under high load.
To avoid global state while still benefiting from connection reuse, you can cache the client instance on self dynamically.
if _check_openai_available():
if not hasattr(self, "_openai_client") or self._openai_client is None:
self._openai_client = _create_openai_client(
api_key=self.config_list[0].get('api_key'),
base_url=self.config_list[0].get('base_url')
)
client = self._openai_client
Greptile SummaryThis PR addresses three separate hardening issues in the wrapper layer: it changes the default server bind address from
Confidence Score: 5/5Safe to merge — all three fixes reduce attack surface without changing any public API contracts. The changes are narrow and well-scoped: default bind addresses are tightened to localhost, the global mutable OpenAI singleton is replaced by per-instance clients, and the scheduler import graph is simplified. No existing callers are broken because the new host parameter has a backward-compatible default and ScheduleParser remains importable from the same scheduler package namespace. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([main / caller]) --> B{use_public?}
B -- Yes --> C[setup_public_url: start ngrok]
C --> D[host = 0.0.0.0\nngrok needs all-interfaces]
D --> G
B -- No --> E[host = caller-supplied\ndefault 127.0.0.1]
E --> F{host == 0.0.0.0?}
F -- Yes --> W[print WARNING:\nexposing to local network]
W --> G
F -- No --> P[print startup URL]
P --> G
G[uvicorn.run app host port]
%%{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([main / caller]) --> B{use_public?}
B -- Yes --> C[setup_public_url: start ngrok]
C --> D[host = 0.0.0.0\nngrok needs all-interfaces]
D --> G
B -- No --> E[host = caller-supplied\ndefault 127.0.0.1]
E --> F{host == 0.0.0.0?}
F -- Yes --> W[print WARNING:\nexposing to local network]
W --> G
F -- No --> P[print startup URL]
P --> G
G[uvicorn.run app host port]
Reviews (3): Last reviewed commit: "fix: wrapper layer architectural fixes (..." | Re-trigger Greptile |
|
@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:
|
|
@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 & Fixes ✅Todo List
Phase 1: Architecture ReviewProtocol-Driven Core: ✅ PASS
Backward Compatibility: ✅ PASS
Performance: ✅ FIXED
Phase 2: Issues FixedApplied fixes in commit
Files Modified
Phase 3: Final Verdict✅ APPROVED - Ready to merge All architectural issues have been resolved:
The PR successfully addresses issue #1602 with minimal, targeted fixes that improve security, performance, and code quality without introducing regressions. |
|
@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. Maintainer triage (22 Jun 2026):
Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude encountered an error —— View job Architecture Review & Conflict Resolution
|
|
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 (2):
|
|
Claude finished @MervinPraison's task —— View job Architecture Review & Conflict Resolution ✅Todo List
Changes Successfully AppliedSuccessfully rebased PR #2085 onto latest main branch! All conflicts resolved while preserving the PR's core fixes:
Files Modified
Architecture Compliance
Final Verdict✅ MERGE READY - All architectural requirements met, conflicts resolved, and reviewer feedback addressed. The PR is clean and ready for CI validation. |
- Fix Gap 1: Remove OpenAI client singleton in auto.py * Replace global _openai_clients cache with per-call client creation * Eliminates thread-safety issues and multi-agent state sharing * Prevents silent resource leaks on cache eviction - Fix Gap 2: Change unsafe 0.0.0.0 default bindings to 127.0.0.1 * Update api/call.py to default to 127.0.0.1 with --host parameter * Add warning when binding to 0.0.0.0 without --public flag * Fix HTTPApproval example to use safe default (127.0.0.1) - Fix Gap 3: Consolidate duplicated ScheduleParser classes * Remove duplicate ScheduleParser from scheduler/base.py * Use single canonical implementation from scheduler/shared.py * Update imports to maintain backward compatibility Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1602-20260503-0822
branch
from
1e1d6f0 to
b03ed4d
Compare
![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/auto.py`:
- Line 56: The comment at line 56 incorrectly describes the OpenAI client
lifecycle as "per-call" when the actual implementation caches clients at the
instance level using self._openai_client and self._async_openai_client
attributes. Update the comment to accurately reflect instance-scoped caching
behavior, specifying that clients are created once on first use
(lazy-initialization) and then reused for all subsequent calls within that same
generator instance, rather than being created fresh on every call or cached
globally across instances.
🪄 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: f3043078-95ad-40eb-b5bc-de37be0e31e8
📒 Files selected for processing (5)
src/praisonai/praisonai/api/call.pysrc/praisonai/praisonai/auto.pysrc/praisonai/praisonai/bots/_http_approval.pysrc/praisonai/praisonai/scheduler/__init__.pysrc/praisonai/praisonai/scheduler/base.py
💤 Files with no reviewable changes (1)
- src/praisonai/praisonai/scheduler/base.py
|
|
||
|
|
||
|
|
||
| # OpenAI client creation removed - create per-call instead of global cache |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 56 comment conflicts with the actual client lifecycle.
At Line 56, the comment says clients are created “per-call,” but this file still caches self._openai_client and self._async_openai_client per generator instance (lazy-init, then reuse). Please reword the comment to reflect instance-scoped caching (no module-level singleton), so the architecture intent is unambiguous.
Suggested wording
-# OpenAI client creation removed - create per-call instead of global cache
+# Removed module-level OpenAI client cache; clients are lazily created and owned per generator instance.🤖 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/auto.py` at line 56, The comment at line 56
incorrectly describes the OpenAI client lifecycle as "per-call" when the actual
implementation caches clients at the instance level using self._openai_client
and self._async_openai_client attributes. Update the comment to accurately
reflect instance-scoped caching behavior, specifying that clients are created
once on first use (lazy-initialization) and then reused for all subsequent calls
within that same generator instance, rather than being created fresh on every
call or cached globally across instances.
- Fix Gap 1: Remove OpenAI client singleton in auto.py * Replace global _openai_clients cache with per-call client creation * Eliminates thread-safety issues and multi-agent state sharing * Prevents silent resource leaks on cache eviction - Fix Gap 2: Change unsafe 0.0.0.0 default bindings to 127.0.0.1 * Update api/call.py to default to 127.0.0.1 with --host parameter * Add warning when binding to 0.0.0.0 without --public flag * Fix HTTPApproval example to use safe default (127.0.0.1) - Fix Gap 3: Consolidate duplicated ScheduleParser classes * Remove duplicate ScheduleParser from scheduler/base.py * Use single canonical implementation from scheduler/shared.py * Update imports to maintain backward compatibility 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 #1602
Auto-opened from
claude/issue-1602-20260503-0822.Summary by CodeRabbit
New Features
--hostCLI argument to configure the server bind address, defaulting to127.0.0.1for local-only access.Documentation