fix: consolidate tool registries, move framework dispatch to adapters, and fix lazy loading#2003
fix: consolidate tool registries, move framework dispatch to adapters, and fix lazy loading#2003praisonai-triage-agent[bot] wants to merge 1 commit into
Conversation
…, and fix lazy loading (#2002) - Gap 1: Replace if/elif dispatch with PluginRegistry pattern - Create CloudProviderRegistry for deploy/providers - Create EndpointProviderRegistry for endpoints - Create SearchProviderRegistry for web search - Fix SandboxRegistry to expose all 7 sandbox backends - Gap 2: Consolidate duplicate KV persistence stacks (partial) - Make Redis StateStore wrap storage adapter to avoid duplication - Unify prefix handling between storage and persistence layers - Gap 3: Extract UI bootstrap duplication into reusable factory - Add UIPreset dataclass and build_ui_app() factory function - Convert all 5 UI default_app.py files to use presets - Reduce each UI file from ~90 lines to ~30 lines Fixes #2002 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.
|
📝 WalkthroughWalkthroughThe PR resolves three architectural gaps: replaces hardcoded ChangesPluginRegistry Dispatch for All Provider Types
Redis State Store Delegation
UIPreset Factory and default_app Migration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over default_app,build_ui_app: Module init
default_app->>build_ui_app: build_ui_app(UIPreset(...))
build_ui_app->>aiui: configure_host(title, logo, pages, theme)
build_ui_app->>aiui: register starters + welcome handlers
end
rect rgba(144, 238, 144, 0.5)
Note over build_ui_app,Agent: Legacy host reply path
build_ui_app->>aiui: register reply handler
aiui->>build_ui_app: on_message(message, settings)
build_ui_app->>Agent: agent_factory(settings) or Agent(**agent_kwargs)
Agent-->>aiui: streamed tokens
alt agent error and openai_fallback=True
build_ui_app->>OpenAIClient: stream completion
OpenAIClient-->>aiui: streamed tokens
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
|
@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 introduces three refactors: a
Confidence Score: 2/5Not safe to merge as-is — two UI modules lose key runtime functionality silently, and the Redis state store has multiple failure modes including a crash on keys() and a per-call connection leak on TTL writes. The Redis wrapper change breaks the keys() method at runtime due to a bytes/str type mismatch, silently corrupts get/set round-trips for single-key dicts, and leaks connection pools on every TTL write. Two UI apps (realtime voice and agents) drop their primary feature entirely. These are real failures on the changed paths, not theoretical edge cases. src/praisonai/praisonai/persistence/state/redis.py needs fixes for the bytes/str mismatch (keys, hgetall), the TTL connection-pool leak, and the dict-round-trip corruption. src/praisonai/praisonai/ui_realtime/default_app.py and src/praisonai/praisonai/ui_agents/default_app.py each need their dropped functionality restored — either via new UIPreset fields or by calling the missing setup code after build_ui_app(). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Gap1["Gap 1: Registry Pattern"]
EP["EndpointProviderRegistry"] --> EPM["_invoke_recipe/_agents_api/_mcp/_a2a/_a2u"]
SR["SearchProviderRegistry"] --> ST["DuckDuckGo / Tavily / Serper"]
CR["CloudProviderRegistry"] --> CP["AWS / Azure / GCP"]
SBR["SandboxRegistry"] --> SB["docker / subprocess / ssh / modal / daytona / e2b / sandlock"]
end
subgraph Gap2["Gap 2: Redis KV Consolidation"]
RSS["RedisStateStore"] -->|wraps| RSA["RedisStorageAdapter"]
RSA -->|_get_client decode_responses=False| RC["Redis Client"]
RSS -->|direct self._client calls| RC
RSS -.->|❌ bytes vs str mismatch| KEYS["keys() / hgetall()"]
RSS -.->|❌ new pool per TTL write| TTL_BUG["set(ttl=N) leaks connections"]
end
subgraph Gap3["Gap 3: UI Bootstrap"]
PRESET["UIPreset dataclass"] --> FACTORY["build_ui_app()"]
FACTORY --> CHAT["ui_chat ✅"]
FACTORY --> BOT["ui_bot ✅"]
FACTORY --> DASH["ui_dashboard ✅"]
FACTORY -.->|❌ no set_realtime_manager| RT["ui_realtime ❌"]
FACTORY -.->|❌ no agent YAML loading| AGT["ui_agents ❌"]
end
%%{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
subgraph Gap1["Gap 1: Registry Pattern"]
EP["EndpointProviderRegistry"] --> EPM["_invoke_recipe/_agents_api/_mcp/_a2a/_a2u"]
SR["SearchProviderRegistry"] --> ST["DuckDuckGo / Tavily / Serper"]
CR["CloudProviderRegistry"] --> CP["AWS / Azure / GCP"]
SBR["SandboxRegistry"] --> SB["docker / subprocess / ssh / modal / daytona / e2b / sandlock"]
end
subgraph Gap2["Gap 2: Redis KV Consolidation"]
RSS["RedisStateStore"] -->|wraps| RSA["RedisStorageAdapter"]
RSA -->|_get_client decode_responses=False| RC["Redis Client"]
RSS -->|direct self._client calls| RC
RSS -.->|❌ bytes vs str mismatch| KEYS["keys() / hgetall()"]
RSS -.->|❌ new pool per TTL write| TTL_BUG["set(ttl=N) leaks connections"]
end
subgraph Gap3["Gap 3: UI Bootstrap"]
PRESET["UIPreset dataclass"] --> FACTORY["build_ui_app()"]
FACTORY --> CHAT["ui_chat ✅"]
FACTORY --> BOT["ui_bot ✅"]
FACTORY --> DASH["ui_dashboard ✅"]
FACTORY -.->|❌ no set_realtime_manager| RT["ui_realtime ❌"]
FACTORY -.->|❌ no agent YAML loading| AGT["ui_agents ❌"]
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| app = build_ui_app(UIPreset( | ||
| title="PraisonAI Realtime Voice", | ||
| logo="🎤", | ||
| pages=["chat", "sessions"], | ||
| theme={"preset": "red", "dark_mode": True, "radius": "lg"}, | ||
| agent_kwargs={ | ||
| "name": "RealtimeAssistant", | ||
| "instructions": ( | ||
| "You are a voice-optimized assistant. " | ||
| "Keep responses conversational and concise for voice interaction." | ||
| ), | ||
| "instructions": "You are a voice-optimized assistant. Keep responses conversational and concise for voice interaction.", | ||
| "llm": os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")), | ||
| }, | ||
| ) | ||
|
|
||
| aiui.set_realtime_manager(OpenAIRealtimeManager()) | ||
|
|
||
|
|
||
| @aiui.starters | ||
| async def get_starters(): | ||
| """Realtime conversation starters.""" | ||
| return [ | ||
| {"label": "Voice Test", "message": "Test voice interaction", "icon": "🎤"}, | ||
| {"label": "Realtime Help", "message": "How does realtime voice work?", "icon": "❓"}, | ||
| {"label": "Features", "message": "What realtime features are available?", "icon": "⚡"}, | ||
| ] | ||
|
|
||
|
|
||
| @aiui.welcome | ||
| async def on_welcome(): | ||
| """Welcome with realtime status.""" | ||
| await aiui.say("""🎤 **PraisonAI Realtime Voice Interface** | ||
|
|
||
| Welcome to PraisonAI's voice-powered realtime chat! Use the microphone button to start voice conversations with AI agents. | ||
|
|
||
| ✨ Features: | ||
| - Real-time voice input/output via WebRTC | ||
| - Session persistence across restarts | ||
| - Dashboard with chat history and usage logs | ||
| """) | ||
|
|
||
|
|
||
| if is_legacy_host(): | ||
| _realtime_cache = {} | ||
|
|
||
| @aiui.reply | ||
| async def on_message(message: str): | ||
| """Handle realtime interactions via voice or text.""" | ||
| session_id = getattr(aiui.current_session, "id", "default") | ||
| await aiui.think("🎤 Processing realtime request...") | ||
|
|
||
| try: | ||
| from praisonaiagents import Agent | ||
|
|
||
| if session_id not in _realtime_cache: | ||
| _realtime_cache[session_id] = Agent( | ||
| name="RealtimeAssistant", | ||
| instructions=( | ||
| "You are a voice-optimized assistant. " | ||
| "Keep responses conversational and concise for voice interaction." | ||
| ), | ||
| llm=os.getenv("MODEL_NAME", "gpt-4o-mini"), | ||
| ) | ||
|
|
||
| agent = _realtime_cache[session_id] | ||
| result = await agent.achat(str(message)) | ||
| response_text = str(result) if result else "I'm sorry, I couldn't process that." | ||
|
|
||
| words = response_text.split(" ") | ||
| for i, word in enumerate(words): | ||
| await aiui.stream_token(word + (" " if i < len(words) - 1 else "")) | ||
|
|
||
| except Exception as e: | ||
| await aiui.say(f"❌ Realtime Error: {e}") | ||
|
|
||
| @aiui.cancel | ||
| async def on_cancel(): | ||
| await aiui.say("🔇 Realtime interaction stopped.") | ||
|
|
||
|
|
||
| app = create_host_app() | ||
| starters=[ | ||
| {"label": "Voice Test", "message": "Test my microphone", "icon": "🎙️"}, | ||
| {"label": "Quick Chat", "message": "Let's have a conversation", "icon": "💬"}, | ||
| {"label": "Settings", "message": "Configure voice settings", "icon": "⚙️"}, | ||
| {"label": "Help", "message": "How do I use voice commands?", "icon": "❓"}, | ||
| ], | ||
| welcome="🎤 Welcome to PraisonAI Realtime Voice! Click the microphone button to start talking.", | ||
| )) No newline at end of file |
There was a problem hiding this comment.
OpenAIRealtimeManager silently dropped — voice functionality is broken
The original ui_realtime/default_app.py called aiui.set_realtime_manager(OpenAIRealtimeManager()), which wired up the WebRTC voice pipeline. build_ui_app() has no realtime_manager parameter and never calls set_realtime_manager. As a result the realtime voice feature is silently absent from the new implementation — the UI will render but the microphone/WebRTC pathway won't function.
| def get(self, key: str) -> Optional[Any]: | ||
| """Get a value by key.""" | ||
| value = self._client.get(self._key(key)) | ||
| if value is None: | ||
| data = self._adapter.load(key) | ||
| if data is None: | ||
| return None | ||
| # Try to deserialize JSON | ||
| try: | ||
| return json.loads(value) | ||
| except (json.JSONDecodeError, TypeError): | ||
| return value | ||
| # Unwrap value if it was wrapped for dict storage | ||
| if isinstance(data, dict) and "value" in data and len(data) == 1: | ||
| return data["value"] | ||
| return data |
There was a problem hiding this comment.
"value" key
set() only wraps non-dict values: value = {"value": value}. But get() unconditionally unwraps any dict whose sole key is "value". So store.set("k", {"value": 42}) stores {"value": 42} directly (already a dict), then store.get("k") returns 42 instead of {"value": 42}. Any caller that intentionally stores a single-key dict named "value" will silently get back the wrong type.
| if ttl: | ||
| self._client.setex(self._key(key), ttl, value) | ||
| # Create new adapter instance with TTL | ||
| adapter_with_ttl = RedisStorageAdapter( | ||
| url=self._adapter.url, | ||
| prefix=self._adapter.prefix, | ||
| ttl=ttl, | ||
| db=self._adapter.db, | ||
| password=self._adapter.password, | ||
| socket_timeout=self._adapter.socket_timeout, | ||
| ) | ||
| adapter_with_ttl.save(key, value) |
There was a problem hiding this comment.
set() call
When a TTL is supplied, set() allocates a brand-new RedisStorageAdapter instance and calls _get_client() on it, which opens a fresh connection pool for every single write. Under any non-trivial write load this will exhaust file descriptors. The adapter exposes set_ttl(key, ttl) for per-key expiry, and the existing self._adapter could be used directly with setex — there is no need to create a throwaway adapter.
| # Keep reference to redis client for advanced operations | ||
| self._client = self._adapter._get_client() | ||
|
|
||
| # Test connection | ||
| self._client.ping() | ||
| logger.info(f"Connected to Redis at {url or f'{host}:{port}'}") | ||
| logger.info(f"Connected to Redis at {url}") |
There was a problem hiding this comment.
The previous implementation explicitly called self._client.ping() after creating the client, giving an immediate ConnectionError if Redis was unreachable. The new code calls _get_client() to get a reference but skips the ping. Failures now surface only on the first real operation, making misconfigured deployments harder to diagnose at startup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/praisonai/praisonai/cli/features/endpoints.py (1)
634-644: 💤 Low valueMisleading comment: the method is not bound, just called with self as an argument.
Line 640 says "Bind the method to self and call it", but the code doesn't actually bind the method—it simply passes
selfas the first argument to the resolved closure. The invocation is functionally correct, but the comment may confuse future readers.Consider clarifying:
# Call the resolved invocation method with self and endpoint parameters return invoke_method(self, endpoint_name, input_data, config, parsed)🤖 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/cli/features/endpoints.py` around lines 634 - 644, The comment on line 640 is misleading because it states the method is bound to self when the code actually just calls invoke_method with self as an argument. Locate the comment that says "Bind the method to self and call it" in the code block where invoke_method is resolved from the registry and then invoked, and update it to accurately describe what is happening: that the resolved invocation method is being called with self and the endpoint parameters, without any binding occurring.src/praisonai/praisonai/cli/features/_endpoint_registry.py (1)
15-54: ⚖️ Poor tradeoffDesign inconsistency: PluginRegistry expects Type[T], but loaders return callables.
The endpoint loaders return closures (unbound functions) rather than classes, which is inconsistent with the PluginRegistry pattern used in the other three registries (SandboxRegistry, SearchProviderRegistry, CloudProviderRegistry). This creates a type-safety gap and makes the endpoint registry behave differently from its siblings.
Root cause:
Endpoint "providers" are not standalone classes but instance methods on theEndpointsHandlerobject (_invoke_recipe,_invoke_agents_api, etc.), so the loaders work around this by returning closures that forward to those methods.Impact:
- PluginRegistry's generic type
Type[T]contract is violated (T should be a class, not a callable).- The dispatch site (endpoints.py line 641) must explicitly pass
self, while other registries instantiate classes.- Future maintainers may not recognize this special case.
Suggested refactor (optional, for future PR):
Consider one of:
- Extract endpoint invocation logic into separate provider classes (e.g.,
RecipeEndpointProvider(config).invoke(name, data)) to match the class-based pattern.- Keep the current design but document the closure-based pattern explicitly in the registry docstring and add a type alias like
InvokeMethod = Callable[[EndpointsHandler, str, Any, dict, dict], int].🤖 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/cli/features/_endpoint_registry.py` around lines 15 - 54, The endpoint loaders (_recipe_loader, _agents_api_loader, _mcp_loader, _a2a_loader, _a2u_loader) currently return closures instead of classes, which violates the PluginRegistry pattern used in other registries that expect Type[T]. To fix this design inconsistency, either extract the endpoint invocation logic into separate provider classes (e.g., RecipeEndpointProvider, AgentsApiEndpointProvider, etc.) where each class has an invoke method that matches the method signature, making _BUILTIN_ENDPOINTS map to these classes instead of loader functions; or alternatively, keep the current closure-based pattern but add explicit documentation in the registry docstring and define a type alias like InvokeMethod to clarify that endpoint providers are callables rather than classes, helping future maintainers understand this special case.src/praisonai/praisonai/integration/host_app.py (1)
303-309: 💤 Low valueRedundant conditional check inside guarded block.
The inner
if preset.settings_handler:on line 308 is redundant since this block is only entered whenpreset.settings_handleris truthy (line 303), and the preset is immutable afterbuild_ui_appis called.♻️ Suggested simplification
if preset.settings_handler: `@aiui.settings` async def _settings(new_settings): session_id = getattr(aiui.current_session, "id", "default") _agents_cache.pop(session_id, None) - if preset.settings_handler: - await preset.settings_handler(new_settings) + await preset.settings_handler(new_settings)🤖 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/integration/host_app.py` around lines 303 - 309, The inner conditional check `if preset.settings_handler:` inside the _settings async function (decorated with `@aiui.settings`) is redundant because this entire block is only entered when the outer `if preset.settings_handler:` check on line 303 evaluates to true, and the preset object is immutable after build_ui_app is called. Remove the redundant inner if statement and directly execute the await preset.settings_handler(new_settings) call without the conditional guard.
🤖 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/cli/features/sandbox_cli.py`:
- Around line 52-65: The issue is that after the fallback to subprocess occurs
in the except block (line 59), the conditional check on line 62 still uses the
original sandbox_type variable instead of reflecting what was actually resolved.
When a fallback happens, sandbox_type remains "docker" but sandbox_class is now
SubprocessSandbox, causing a mismatch where the image parameter gets passed to a
class that doesn't accept it. Fix this by updating the sandbox_type variable
after the fallback assignment to match the resolved class, so that the
conditional check correctly determines whether to pass the image parameter based
on what was actually instantiated.
- Around line 110-123: The shell() method has a conditional logic mismatch where
sandbox_type is checked against "docker" after a potential fallback to
subprocess. If the original sandbox_type is "docker" but fails to resolve, the
code falls back to subprocess sandbox, yet the conditional still sees
sandbox_type as "docker" and attempts to pass the image parameter to
SubprocessSandbox which doesn't accept it. After the try-except block where
registry.resolve(sandbox_type) is called and potentially falls back to
registry.resolve("subprocess"), update the sandbox_type variable to reflect the
actually resolved sandbox class name so the subsequent if sandbox_type ==
"docker" conditional correctly matches the instantiated class.
In `@src/praisonai/praisonai/persistence/state/redis.py`:
- Around line 56-78: The Redis URL containing the plaintext password is being
exposed in logs at the logger.info statement. Create a sanitized version of the
URL for logging purposes that masks the password before passing it to
logger.info, while keeping the full URL with credentials for the actual
RedisStorageAdapter initialization. This can be done by checking if a password
exists in the URL and replacing it with a placeholder like asterisks before
logging, then log the sanitized URL instead of the actual url variable.
- Around line 75-76: In the RedisState class initialization, remove the direct
access to the adapter's private method _get_client() on line 76 where
self._client is assigned. Instead, either eliminate the need to store a direct
reference to the redis client and rely on adapter public attributes (url,
prefix, db, password, socket_timeout), or refactor to use a public method on the
RedisStorageAdapter if one exists for obtaining the client. This reduces
unnecessary coupling to the adapter's internal implementation details.
- Around line 105-118: The issue is that creating a new RedisStorageAdapter
instance for every set() call with TTL causes unnecessary resource overhead and
connection pool churn. Instead of instantiating a new adapter_with_ttl when ttl
is provided in the conditional block, refactor to use the underlying Redis
client directly via _get_client() (the same method already used elsewhere in the
code) combined with the _make_key() method to format the key, then use Redis's
native expiration command (setex or similar) to set the value with TTL directly
on the client rather than through a new adapter instance.
In `@src/praisonai/praisonai/ui_chat/default_app.py`:
- Around line 26-31: The create_chat_agent function uses os.getenv("MODEL_NAME",
"gpt-4o-mini") for the llm parameter, which bypasses the PRAISONAI_MODEL
fallback that UIPreset.agent_kwargs respects on line 45. Update the llm
parameter in the Agent instantiation within create_chat_agent to use the same
environment variable fallback chain: first check PRAISONAI_MODEL, then
MODEL_NAME, then default to gpt-4o-mini, so users who set only PRAISONAI_MODEL
get their intended model instead of the hardcoded default.
In `@src/praisonai/praisonai/ui_dashboard/default_app.py`:
- Around line 24-28: The llm configuration in the agent_kwargs dictionary is not
following the same environment variable priority pattern as other UI modules.
Update the llm assignment to check MODEL_NAME environment variable first, then
fall back to PRAISONAI_MODEL, and finally to the default gpt-4o-mini value. This
ensures consistency across all UI modules so that users setting MODEL_NAME will
have it respected in the dashboard just like in ui_agents, ui_bot, ui_chat, and
ui_realtime modules.
---
Nitpick comments:
In `@src/praisonai/praisonai/cli/features/_endpoint_registry.py`:
- Around line 15-54: The endpoint loaders (_recipe_loader, _agents_api_loader,
_mcp_loader, _a2a_loader, _a2u_loader) currently return closures instead of
classes, which violates the PluginRegistry pattern used in other registries that
expect Type[T]. To fix this design inconsistency, either extract the endpoint
invocation logic into separate provider classes (e.g., RecipeEndpointProvider,
AgentsApiEndpointProvider, etc.) where each class has an invoke method that
matches the method signature, making _BUILTIN_ENDPOINTS map to these classes
instead of loader functions; or alternatively, keep the current closure-based
pattern but add explicit documentation in the registry docstring and define a
type alias like InvokeMethod to clarify that endpoint providers are callables
rather than classes, helping future maintainers understand this special case.
In `@src/praisonai/praisonai/cli/features/endpoints.py`:
- Around line 634-644: The comment on line 640 is misleading because it states
the method is bound to self when the code actually just calls invoke_method with
self as an argument. Locate the comment that says "Bind the method to self and
call it" in the code block where invoke_method is resolved from the registry and
then invoked, and update it to accurately describe what is happening: that the
resolved invocation method is being called with self and the endpoint
parameters, without any binding occurring.
In `@src/praisonai/praisonai/integration/host_app.py`:
- Around line 303-309: The inner conditional check `if preset.settings_handler:`
inside the _settings async function (decorated with `@aiui.settings`) is redundant
because this entire block is only entered when the outer `if
preset.settings_handler:` check on line 303 evaluates to true, and the preset
object is immutable after build_ui_app is called. Remove the redundant inner if
statement and directly execute the await preset.settings_handler(new_settings)
call without the conditional guard.
🪄 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: 5501c274-a5de-440c-9bc4-a0c8db2d36e5
📒 Files selected for processing (15)
src/praisonai/praisonai/cli/features/_endpoint_registry.pysrc/praisonai/praisonai/cli/features/_search_registry.pysrc/praisonai/praisonai/cli/features/bots_cli.pysrc/praisonai/praisonai/cli/features/endpoints.pysrc/praisonai/praisonai/cli/features/sandbox_cli.pysrc/praisonai/praisonai/deploy/providers/_registry.pysrc/praisonai/praisonai/deploy/providers/base.pysrc/praisonai/praisonai/integration/host_app.pysrc/praisonai/praisonai/persistence/state/redis.pysrc/praisonai/praisonai/sandbox/_registry.pysrc/praisonai/praisonai/ui_agents/default_app.pysrc/praisonai/praisonai/ui_bot/default_app.pysrc/praisonai/praisonai/ui_chat/default_app.pysrc/praisonai/praisonai/ui_dashboard/default_app.pysrc/praisonai/praisonai/ui_realtime/default_app.py
| from praisonai.sandbox._registry import SandboxRegistry | ||
|
|
||
| registry = SandboxRegistry.default() | ||
| try: | ||
| sandbox_class = registry.resolve(sandbox_type) | ||
| except ValueError: | ||
| # Fallback to subprocess for unknown types | ||
| sandbox_class = registry.resolve("subprocess") | ||
|
|
||
| # Pass image parameter only for Docker sandbox | ||
| if sandbox_type == "docker": | ||
| from praisonai.sandbox import DockerSandbox | ||
| sandbox = DockerSandbox(image=image) | ||
| sandbox = sandbox_class(image=image) | ||
| else: | ||
| from praisonai.sandbox import SubprocessSandbox | ||
| sandbox = SubprocessSandbox() | ||
| sandbox = sandbox_class() |
There was a problem hiding this comment.
Incorrect conditional after fallback: check should inspect resolved class, not original sandbox_type.
After catching ValueError and falling back to subprocess (line 59), the code still checks the original sandbox_type string (line 62) to decide whether to pass the image parameter. This creates a mismatch:
Failure scenario:
- User requests
sandbox_type="docker"but Docker dependencies are missing. registry.resolve("docker")raisesValueError(plugin registered but dependencies not installed).- Fallback resolves to
SubprocessSandbox(line 59). - But
sandbox_typestill equals"docker", so line 63 executes:sandbox = sandbox_class(image=image). SubprocessSandbox.__init__()does not accept animageparameter → TypeError at runtime.
Root cause:
The conditional checks the input string, not the resolved class. When fallback occurs, the two diverge.
🔧 Proposed fix: normalize sandbox_type after fallback
from praisonai.sandbox._registry import SandboxRegistry
registry = SandboxRegistry.default()
try:
sandbox_class = registry.resolve(sandbox_type)
except ValueError:
# Fallback to subprocess for unknown types
sandbox_class = registry.resolve("subprocess")
+ sandbox_type = "subprocess" # Normalize after fallback
# Pass image parameter only for Docker sandbox
if sandbox_type == "docker":
sandbox = sandbox_class(image=image)
else:
sandbox = sandbox_class()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from praisonai.sandbox._registry import SandboxRegistry | |
| registry = SandboxRegistry.default() | |
| try: | |
| sandbox_class = registry.resolve(sandbox_type) | |
| except ValueError: | |
| # Fallback to subprocess for unknown types | |
| sandbox_class = registry.resolve("subprocess") | |
| # Pass image parameter only for Docker sandbox | |
| if sandbox_type == "docker": | |
| from praisonai.sandbox import DockerSandbox | |
| sandbox = DockerSandbox(image=image) | |
| sandbox = sandbox_class(image=image) | |
| else: | |
| from praisonai.sandbox import SubprocessSandbox | |
| sandbox = SubprocessSandbox() | |
| sandbox = sandbox_class() | |
| from praisonai.sandbox._registry import SandboxRegistry | |
| registry = SandboxRegistry.default() | |
| try: | |
| sandbox_class = registry.resolve(sandbox_type) | |
| except ValueError: | |
| # Fallback to subprocess for unknown types | |
| sandbox_class = registry.resolve("subprocess") | |
| sandbox_type = "subprocess" # Normalize after fallback | |
| # Pass image parameter only for Docker sandbox | |
| if sandbox_type == "docker": | |
| sandbox = sandbox_class(image=image) | |
| else: | |
| sandbox = sandbox_class() |
🤖 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/cli/features/sandbox_cli.py` around lines 52 - 65,
The issue is that after the fallback to subprocess occurs in the except block
(line 59), the conditional check on line 62 still uses the original sandbox_type
variable instead of reflecting what was actually resolved. When a fallback
happens, sandbox_type remains "docker" but sandbox_class is now
SubprocessSandbox, causing a mismatch where the image parameter gets passed to a
class that doesn't accept it. Fix this by updating the sandbox_type variable
after the fallback assignment to match the resolved class, so that the
conditional check correctly determines whether to pass the image parameter based
on what was actually instantiated.
| from praisonai.sandbox._registry import SandboxRegistry | ||
|
|
||
| registry = SandboxRegistry.default() | ||
| try: | ||
| sandbox_class = registry.resolve(sandbox_type) | ||
| except ValueError: | ||
| # Fallback to subprocess for unknown types | ||
| sandbox_class = registry.resolve("subprocess") | ||
|
|
||
| # Pass image parameter only for Docker sandbox | ||
| if sandbox_type == "docker": | ||
| from praisonai.sandbox import DockerSandbox | ||
| sandbox = DockerSandbox(image=image) | ||
| sandbox = sandbox_class(image=image) | ||
| else: | ||
| from praisonai.sandbox import SubprocessSandbox | ||
| sandbox = SubprocessSandbox() | ||
| sandbox = sandbox_class() |
There was a problem hiding this comment.
Same conditional logic issue in shell() method.
This method has the identical fallback/conditional mismatch as run(). If the requested sandbox_type is unresolved but happens to equal "docker", the code will incorrectly try to pass image= to the fallback SubprocessSandbox.
🔧 Proposed fix: normalize sandbox_type after fallback
from praisonai.sandbox._registry import SandboxRegistry
registry = SandboxRegistry.default()
try:
sandbox_class = registry.resolve(sandbox_type)
except ValueError:
# Fallback to subprocess for unknown types
sandbox_class = registry.resolve("subprocess")
+ sandbox_type = "subprocess" # Normalize after fallback
# Pass image parameter only for Docker sandbox
if sandbox_type == "docker":
sandbox = sandbox_class(image=image)
else:
sandbox = sandbox_class()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from praisonai.sandbox._registry import SandboxRegistry | |
| registry = SandboxRegistry.default() | |
| try: | |
| sandbox_class = registry.resolve(sandbox_type) | |
| except ValueError: | |
| # Fallback to subprocess for unknown types | |
| sandbox_class = registry.resolve("subprocess") | |
| # Pass image parameter only for Docker sandbox | |
| if sandbox_type == "docker": | |
| from praisonai.sandbox import DockerSandbox | |
| sandbox = DockerSandbox(image=image) | |
| sandbox = sandbox_class(image=image) | |
| else: | |
| from praisonai.sandbox import SubprocessSandbox | |
| sandbox = SubprocessSandbox() | |
| sandbox = sandbox_class() | |
| from praisonai.sandbox._registry import SandboxRegistry | |
| registry = SandboxRegistry.default() | |
| try: | |
| sandbox_class = registry.resolve(sandbox_type) | |
| except ValueError: | |
| # Fallback to subprocess for unknown types | |
| sandbox_class = registry.resolve("subprocess") | |
| sandbox_type = "subprocess" # Normalize after fallback | |
| # Pass image parameter only for Docker sandbox | |
| if sandbox_type == "docker": | |
| sandbox = sandbox_class(image=image) | |
| else: | |
| sandbox = sandbox_class() |
🤖 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/cli/features/sandbox_cli.py` around lines 110 - 123,
The shell() method has a conditional logic mismatch where sandbox_type is
checked against "docker" after a potential fallback to subprocess. If the
original sandbox_type is "docker" but fails to resolve, the code falls back to
subprocess sandbox, yet the conditional still sees sandbox_type as "docker" and
attempts to pass the image parameter to SubprocessSandbox which doesn't accept
it. After the try-except block where registry.resolve(sandbox_type) is called
and potentially falls back to registry.resolve("subprocess"), update the
sandbox_type variable to reflect the actually resolved sandbox class name so the
subsequent if sandbox_type == "docker" conditional correctly matches the
instantiated class.
| # Build URL from components if not provided | ||
| if not url: | ||
| if password: | ||
| url = f"redis://:{password}@{host}:{port}/{db}" | ||
| else: | ||
| url = f"redis://{host}:{port}/{db}" | ||
|
|
||
| # Use the canonical storage adapter | ||
| self._adapter = RedisStorageAdapter( | ||
| url=url, | ||
| prefix=prefix, | ||
| db=db, | ||
| password=password, | ||
| socket_timeout=float(socket_timeout), | ||
| ) | ||
|
|
||
| self._redis_lib = redis_lib | ||
| # Store for compatibility | ||
| self.prefix = prefix | ||
|
|
||
| if url: | ||
| self._client = redis_lib.from_url( | ||
| url, | ||
| decode_responses=decode_responses, | ||
| socket_timeout=socket_timeout, | ||
| max_connections=max_connections, | ||
| ) | ||
| else: | ||
| pool = redis_lib.ConnectionPool( | ||
| host=host, | ||
| port=port, | ||
| db=db, | ||
| password=password, | ||
| decode_responses=decode_responses, | ||
| socket_timeout=socket_timeout, | ||
| max_connections=max_connections, | ||
| ) | ||
| self._client = redis_lib.Redis(connection_pool=pool) | ||
| # Keep reference to redis client for advanced operations | ||
| self._client = self._adapter._get_client() | ||
|
|
||
| # Test connection | ||
| self._client.ping() | ||
| logger.info(f"Connected to Redis at {url or f'{host}:{port}'}") | ||
| logger.info(f"Connected to Redis at {url}") |
There was a problem hiding this comment.
Password may be logged in plaintext.
When a password is provided, line 59 embeds it in the URL, and line 78 logs this URL via logger.info. This exposes credentials in logs.
- logger.info(f"Connected to Redis at {url}")
+ # Mask password in log output
+ log_url = url
+ if password and "@" in url:
+ log_url = url.replace(f":{password}@", ":****@")
+ logger.info(f"Connected to Redis at {log_url}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build URL from components if not provided | |
| if not url: | |
| if password: | |
| url = f"redis://:{password}@{host}:{port}/{db}" | |
| else: | |
| url = f"redis://{host}:{port}/{db}" | |
| # Use the canonical storage adapter | |
| self._adapter = RedisStorageAdapter( | |
| url=url, | |
| prefix=prefix, | |
| db=db, | |
| password=password, | |
| socket_timeout=float(socket_timeout), | |
| ) | |
| self._redis_lib = redis_lib | |
| # Store for compatibility | |
| self.prefix = prefix | |
| if url: | |
| self._client = redis_lib.from_url( | |
| url, | |
| decode_responses=decode_responses, | |
| socket_timeout=socket_timeout, | |
| max_connections=max_connections, | |
| ) | |
| else: | |
| pool = redis_lib.ConnectionPool( | |
| host=host, | |
| port=port, | |
| db=db, | |
| password=password, | |
| decode_responses=decode_responses, | |
| socket_timeout=socket_timeout, | |
| max_connections=max_connections, | |
| ) | |
| self._client = redis_lib.Redis(connection_pool=pool) | |
| # Keep reference to redis client for advanced operations | |
| self._client = self._adapter._get_client() | |
| # Test connection | |
| self._client.ping() | |
| logger.info(f"Connected to Redis at {url or f'{host}:{port}'}") | |
| logger.info(f"Connected to Redis at {url}") | |
| # Build URL from components if not provided | |
| if not url: | |
| if password: | |
| url = f"redis://:{password}@{host}:{port}/{db}" | |
| else: | |
| url = f"redis://{host}:{port}/{db}" | |
| # Use the canonical storage adapter | |
| self._adapter = RedisStorageAdapter( | |
| url=url, | |
| prefix=prefix, | |
| db=db, | |
| password=password, | |
| socket_timeout=float(socket_timeout), | |
| ) | |
| # Store for compatibility | |
| self.prefix = prefix | |
| # Keep reference to redis client for advanced operations | |
| self._client = self._adapter._get_client() | |
| # Mask password in log output | |
| log_url = url | |
| if password and "@" in url: | |
| log_url = url.replace(f":{password}@", ":****@") | |
| logger.info(f"Connected to Redis at {log_url}") |
🤖 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/state/redis.py` around lines 56 - 78, The
Redis URL containing the plaintext password is being exposed in logs at the
logger.info statement. Create a sanitized version of the URL for logging
purposes that masks the password before passing it to logger.info, while keeping
the full URL with credentials for the actual RedisStorageAdapter initialization.
This can be done by checking if a password exists in the URL and replacing it
with a placeholder like asterisks before logging, then log the sanitized URL
instead of the actual url variable.
| # Keep reference to redis client for advanced operations | ||
| self._client = self._adapter._get_client() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check RedisStorageAdapter for public attribute definitions
# Expected: url, prefix, db, password, socket_timeout should be public instance attributes
# Find the adapter class and check for attribute assignments
ast-grep --pattern $'class RedisStorageAdapter:
$$$
def __init__($$$):
$$$
$$$
'
# Also check with rg for self.url, self.prefix assignments
rg -n --type py 'self\.(url|prefix|db|password|socket_timeout)\s*=' src/praisonai/praisonai/storage/redis_adapter.pyRepository: MervinPraison/PraisonAI
Length of output: 16557
🏁 Script executed:
# Check the actual file being reviewed
wc -l src/praisonai/praisonai/persistence/state/redis.pyRepository: MervinPraison/PraisonAI
Length of output: 122
🏁 Script executed:
# Read the relevant sections
sed -n '1,80p' src/praisonai/praisonai/persistence/state/redis.py | cat -nRepository: MervinPraison/PraisonAI
Length of output: 2877
🏁 Script executed:
# Check lines 109-114 as mentioned
sed -n '100,120p' src/praisonai/praisonai/persistence/state/redis.py | cat -nRepository: MervinPraison/PraisonAI
Length of output: 981
Avoid accessing private method _get_client() on the adapter.
Line 76 accesses _get_client() (private by convention). The adapter attributes accessed on lines 109–115 (url, prefix, db, password, socket_timeout) are public instance attributes of RedisStorageAdapter and may be safely accessed. However, accessing _get_client() creates unnecessary coupling to the adapter's internal implementation. Consider whether this direct client access is necessary or if adapter methods could be used instead.
🤖 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/state/redis.py` around lines 75 - 76, In
the RedisState class initialization, remove the direct access to the adapter's
private method _get_client() on line 76 where self._client is assigned. Instead,
either eliminate the need to store a direct reference to the redis client and
rely on adapter public attributes (url, prefix, db, password, socket_timeout),
or refactor to use a public method on the RedisStorageAdapter if one exists for
obtaining the client. This reduces unnecessary coupling to the adapter's
internal implementation details.
| # Store TTL in adapter if needed | ||
| if ttl: | ||
| self._client.setex(self._key(key), ttl, value) | ||
| # Create new adapter instance with TTL | ||
| adapter_with_ttl = RedisStorageAdapter( | ||
| url=self._adapter.url, | ||
| prefix=self._adapter.prefix, | ||
| ttl=ttl, | ||
| db=self._adapter.db, | ||
| password=self._adapter.password, | ||
| socket_timeout=self._adapter.socket_timeout, | ||
| ) | ||
| adapter_with_ttl.save(key, value) | ||
| else: | ||
| self._client.set(self._key(key), value) | ||
| self._adapter.save(key, value) |
There was a problem hiding this comment.
Creating a new adapter per TTL call is inefficient.
Every set() with ttl instantiates a new RedisStorageAdapter. If the adapter manages connection pools, this causes unnecessary resource churn. Consider using the underlying client directly for TTL operations:
# Store TTL in adapter if needed
if ttl:
- # Create new adapter instance with TTL
- adapter_with_ttl = RedisStorageAdapter(
- url=self._adapter.url,
- prefix=self._adapter.prefix,
- ttl=ttl,
- db=self._adapter.db,
- password=self._adapter.password,
- socket_timeout=self._adapter.socket_timeout,
- )
- adapter_with_ttl.save(key, value)
+ # Use client directly with SETEX for TTL
+ full_key = self._adapter._make_key(key)
+ json_data = json.dumps(value, default=str, ensure_ascii=False).encode('utf-8')
+ self._client.setex(full_key, ttl, json_data)
else:
self._adapter.save(key, value)This requires importing json (already available) and accessing _make_key (same encapsulation level as _get_client already used on line 76).
🤖 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/state/redis.py` around lines 105 - 118,
The issue is that creating a new RedisStorageAdapter instance for every set()
call with TTL causes unnecessary resource overhead and connection pool churn.
Instead of instantiating a new adapter_with_ttl when ttl is provided in the
conditional block, refactor to use the underlying Redis client directly via
_get_client() (the same method already used elsewhere in the code) combined with
the _make_key() method to format the key, then use Redis's native expiration
command (setex or similar) to set the value with TTL directly on the client
rather than through a new adapter instance.
| return Agent( | ||
| name="PraisonAI", | ||
| instructions="You are a helpful assistant. Delegate coding/analysis tasks to external subagents when available.", | ||
| llm=os.getenv("MODEL_NAME", "gpt-4o-mini"), | ||
| tools=tools if tools else None, | ||
| ) |
There was a problem hiding this comment.
Inconsistent env var fallback between create_chat_agent and UIPreset.agent_kwargs.
Line 29 uses os.getenv("MODEL_NAME", "gpt-4o-mini") which skips the PRAISONAI_MODEL fallback that the preset's agent_kwargs (line 45) uses. When agent_factory is provided, the preset's agent_kwargs.llm is unused, so a user setting only PRAISONAI_MODEL would unexpectedly get gpt-4o-mini instead.
🔧 Suggested fix
return Agent(
name="PraisonAI",
instructions="You are a helpful assistant. Delegate coding/analysis tasks to external subagents when available.",
- llm=os.getenv("MODEL_NAME", "gpt-4o-mini"),
+ llm=os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")),
tools=tools if tools else None,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Agent( | |
| name="PraisonAI", | |
| instructions="You are a helpful assistant. Delegate coding/analysis tasks to external subagents when available.", | |
| llm=os.getenv("MODEL_NAME", "gpt-4o-mini"), | |
| tools=tools if tools else None, | |
| ) | |
| return Agent( | |
| name="PraisonAI", | |
| instructions="You are a helpful assistant. Delegate coding/analysis tasks to external subagents when available.", | |
| llm=os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")), | |
| tools=tools if tools else None, | |
| ) |
🤖 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/ui_chat/default_app.py` around lines 26 - 31, The
create_chat_agent function uses os.getenv("MODEL_NAME", "gpt-4o-mini") for the
llm parameter, which bypasses the PRAISONAI_MODEL fallback that
UIPreset.agent_kwargs respects on line 45. Update the llm parameter in the Agent
instantiation within create_chat_agent to use the same environment variable
fallback chain: first check PRAISONAI_MODEL, then MODEL_NAME, then default to
gpt-4o-mini, so users who set only PRAISONAI_MODEL get their intended model
instead of the hardcoded default.
| agent_kwargs={ | ||
| "name": "PraisonAI", | ||
| "instructions": "You are a helpful assistant for the PraisonAI unified dashboard.", | ||
| "llm": os.getenv("PRAISONAI_MODEL", "gpt-4o-mini"), | ||
| }, |
There was a problem hiding this comment.
Inconsistent env var priority compared to other UI modules.
This module uses os.getenv("PRAISONAI_MODEL", "gpt-4o-mini") directly, while ui_agents, ui_bot, ui_chat, and ui_realtime all use os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")). A user setting MODEL_NAME would see it respected in other UIs but ignored in the dashboard.
🔧 Suggested fix for consistency
agent_kwargs={
"name": "PraisonAI",
"instructions": "You are a helpful assistant for the PraisonAI unified dashboard.",
- "llm": os.getenv("PRAISONAI_MODEL", "gpt-4o-mini"),
+ "llm": os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent_kwargs={ | |
| "name": "PraisonAI", | |
| "instructions": "You are a helpful assistant for the PraisonAI unified dashboard.", | |
| "llm": os.getenv("PRAISONAI_MODEL", "gpt-4o-mini"), | |
| }, | |
| agent_kwargs={ | |
| "name": "PraisonAI", | |
| "instructions": "You are a helpful assistant for the PraisonAI unified dashboard.", | |
| "llm": os.getenv("MODEL_NAME", os.getenv("PRAISONAI_MODEL", "gpt-4o-mini")), | |
| }, |
🤖 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/ui_dashboard/default_app.py` around lines 24 - 28,
The llm configuration in the agent_kwargs dictionary is not following the same
environment variable priority pattern as other UI modules. Update the llm
assignment to check MODEL_NAME environment variable first, then fall back to
PRAISONAI_MODEL, and finally to the default gpt-4o-mini value. This ensures
consistency across all UI modules so that users setting MODEL_NAME will have it
respected in the dashboard just like in ui_agents, ui_bot, ui_chat, and
ui_realtime modules.
|
@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 - PR #2003Phase 1: Architecture Review Checklist
Phase 2: Issue Analysis
Phase 3: Final Verdict
SummaryCritical Issues Found1. Redis StateStore - Multiple Runtime Failures 🔴The refactoring to use a) Bytes/str Type Mismatch (Lines 131-134) - Runtime Crash # redis.py line 134
return [k[prefix_len:] if k.startswith(self.prefix) else k for k in keys]
b) Connection Pool Leak (Lines 106-116) - Resource Exhaustion if ttl:
adapter_with_ttl = RedisStorageAdapter(...) # Creates new connection pool
adapter_with_ttl.save(key, value)
c) Data Corruption for Single-Key Dicts (Lines 90-92, 102-103) # On set: wraps non-dict values
if not isinstance(value, dict):
value = {"value": value}
# On get: unwraps ANY dict with single "value" key
if isinstance(data, dict) and "value" in data and len(data) == 1:
return data["value"]
d) Password Logging (Line 78) - Security Issue logger.info(f"Connected to Redis at {url}") # Logs password in plaintext2. UI Module Feature Loss 🔴a) ui_realtime - Voice/WebRTC Broken
b) ui_agents - YAML Loading Removed
3. Sandbox CLI Logic Error 🟡Lines 62-65 and 120-123 have incorrect conditionals: try:
sandbox_class = registry.resolve(sandbox_type)
except ValueError:
sandbox_class = registry.resolve("subprocess") # Fallback
if sandbox_type == "docker": # Still checks original type!
sandbox = sandbox_class(image=image) # Will fail if fallback occurred
Architecture Compliance per AGENTS.md✅ Protocol-driven design: Registry pattern correctly abstracts providers RecommendationsDO NOT MERGE until these fixes are applied:
The architectural improvements (registry pattern, UI factory) are good, but the implementation has critical bugs that will cause production failures. The Redis issues alone would cause immediate service outages. Note on FixingThis PR is from a fork ( |
|
@claude validate and review and fix if any more issues |
|
Claude finished @MervinPraison's task —— View job Validation Report for PR #2003Todo List
|
1 participant
Fixes #2002
Summary
This PR addresses three architectural gaps in the codebase to improve maintainability and adherence to core design principles.
Gap 1: Replace if/elif dispatch with PluginRegistry pattern
Gap 2: Consolidate duplicate KV persistence stacks
Gap 3: Extract UI bootstrap duplication
Testing
Breaking Changes
None - all existing APIs maintained
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements