feat: agent god class decomposition - create mixin framework#1243
feat: agent god class decomposition - create mixin framework#1243MervinPraison wants to merge 1 commit into
Conversation
- Create 3 new mixins for agent decomposition: * chat_mixin.py - Chat, LLM, streaming methods (~1500 lines planned) * execution_mixin.py - Run, start, autonomous execution (~1200 lines planned) * memory_mixin.py - Cache, history, state management (~500 lines planned) - Update Agent class to inherit from all mixins - Maintain 100% backward compatibility - Follow protocol-driven design principles from AGENTS.md This establishes the mixin framework for decomposing agent.py from 8,915 lines to ≤5000 lines. Actual method implementations need to be moved in future work. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Review Summary by QodoCreate mixin framework for agent god class decomposition
WalkthroughsDescription• Create 3 new mixin classes for agent decomposition framework - ChatMixin: Chat, LLM, streaming, and response processing methods - ExecutionMixin: Run, start, autonomous execution, and task lifecycle - MemoryMixin: Chat history, caching, and state management • Update Agent class to inherit from all three new mixins • Maintain 100% backward compatibility with existing code • Establish foundation for decomposing agent.py from 8,915 to ≤5,000 lines Diagramflowchart LR
agent["Agent Class<br/>8,915 lines"]
chat["ChatMixin<br/>Chat & LLM"]
exec["ExecutionMixin<br/>Run & Execution"]
mem["MemoryMixin<br/>History & Cache"]
old["Existing Mixins<br/>Tool, Chat, Session"]
agent -- "inherits from" --> chat
agent -- "inherits from" --> exec
agent -- "inherits from" --> mem
agent -- "inherits from" --> old
chat -- "placeholder methods" --> impl["TODO: Move implementations<br/>from agent.py"]
exec -- "placeholder methods" --> impl
mem -- "placeholder methods" --> impl
File Changes1. src/praisonai-agents/praisonaiagents/agent/agent.py
|
Code Review by Qodo
1. Mixins not publicly exported
|
📝 WalkthroughWalkthroughThis PR introduces three new mixin classes—ChatMixin, ExecutionMixin, and MemoryMixin—and refactors the Agent class to inherit from them. The mixins decompose chat interactions, execution lifecycle, and memory management into separate components, with Agent updated to include these new base classes alongside existing mixins. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
⚔️ Resolve merge conflicts
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 |
There was a problem hiding this comment.
Pull request overview
Introduces a new mixin-based framework to decompose the large Agent “god class” into focused concerns (chat, execution, memory) while keeping the existing Agent entrypoint and inheritance structure.
Changes:
- Added
ChatMixin,ExecutionMixin, andMemoryMixinmodules as scaffolding for future method migration fromagent.py. - Updated
Agentto inherit from the new mixins to establish the decomposition framework. - Added initial placeholder implementations and helpers (e.g., lazy imports, basic chat history state).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/memory_mixin.py | Adds memory/history/cache mixin scaffold and basic chat history utilities. |
| src/praisonai-agents/praisonaiagents/agent/execution_mixin.py | Adds execution mixin scaffold with placeholder sync/async run/start APIs and helpers. |
| src/praisonai-agents/praisonaiagents/agent/chat_mixin.py | Adds chat/LLM mixin scaffold, including lazy import helpers and trace wrapper. |
| src/praisonai-agents/praisonaiagents/agent/agent.py | Wires the new mixins into the Agent inheritance list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise error | ||
|
|
||
| def _validate_execution_params(self, **kwargs) -> Dict[str, Any]: |
There was a problem hiding this comment.
raise error will replace the original traceback with the current frame, making debugging harder. Use bare raise to preserve the original traceback when re-throwing inside an exception handler.
| raise error | |
| def _validate_execution_params(self, **kwargs) -> Dict[str, Any]: | |
| raise error.with_traceback(error.__traceback__) | |
| def _validate_execution_params(self, **kwargs) -> Dict[str, Any]) -> Dict[str, Any]: |
| def run_until(self, max_iterations: int = 10, max_time: Optional[float] = None, | ||
| condition: Optional[callable] = None, **kwargs) -> Any: |
There was a problem hiding this comment.
Using callable as a type isn’t a useful annotation for type checkers. Prefer Callable[..., bool] (or a more specific signature) from typing for condition.
| async def run_until_async(self, max_iterations: int = 10, max_time: Optional[float] = None, | ||
| condition: Optional[callable] = None, **kwargs) -> Any: |
There was a problem hiding this comment.
Using callable as a type isn’t a useful annotation for type checkers. Prefer Callable[..., bool] (or a more specific signature) from typing for condition.
| import os | ||
| import time | ||
| import json | ||
| import asyncio | ||
| import threading | ||
| from typing import List, Optional, Any, Dict, Union, Callable, Generator |
There was a problem hiding this comment.
This module has several unused imports (e.g., os, time, json, asyncio, and likely Callable/Generator depending on future additions). If the repo enforces linting (ruff/flake8), this will fail CI; remove unused imports or use them in the current scaffold.
| import os | |
| import time | |
| import json | |
| import asyncio | |
| import threading | |
| from typing import List, Optional, Any, Dict, Union, Callable, Generator | |
| import threading |
| import asyncio | ||
| import threading | ||
| from typing import Any, Dict, List, Optional, Union, Generator |
There was a problem hiding this comment.
asyncio, threading, and List/Union/Generator appear unused in the current scaffold. If linting is enforced, remove unused imports now to avoid CI failures while the mixin is still a placeholder.
| import asyncio | |
| import threading | |
| from typing import Any, Dict, List, Optional, Union, Generator | |
| from typing import Any, Dict, Optional, Union, Generator |
| import threading | ||
| from typing import Any, Dict, List, Optional, Union |
There was a problem hiding this comment.
threading, Optional, and Union appear unused in this file as written. If linting is enforced, remove unused imports to keep the new module clean while it’s scaffolded.
| import threading | |
| from typing import Any, Dict, List, Optional, Union | |
| from typing import Any, Dict, List |
| - State management and cleanup | ||
| """ | ||
|
|
||
| def _cache_put(self, cache_dict: Dict[str, Any], key: str, value: Any) -> None: |
There was a problem hiding this comment.
Both _cache_put and _cache_get type cache_dict as Dict[str, Any] but then explicitly handle None. Align the signature with behavior (e.g., Optional[MutableMapping[str, Any]]) or drop the None handling if callers should always pass a dict.
| if cache_dict is not None: | ||
| cache_dict[key] = value | ||
|
|
||
| def _cache_get(self, cache_dict: Dict[str, Any], key: str) -> Any: |
There was a problem hiding this comment.
Both _cache_put and _cache_get type cache_dict as Dict[str, Any] but then explicitly handle None. Align the signature with behavior (e.g., Optional[MutableMapping[str, Any]]) or drop the None handling if callers should always pass a dict.
| # - Cache hit/miss tracking | ||
| # - Expiration checking | ||
|
|
||
| if cache_dict is not None: |
There was a problem hiding this comment.
Both _cache_put and _cache_get type cache_dict as Dict[str, Any] but then explicitly handle None. Align the signature with behavior (e.g., Optional[MutableMapping[str, Any]]) or drop the None handling if callers should always pass a dict.
|
|
||
| return { | ||
| 'chat_history_messages': chat_history_size, | ||
| 'chat_history_size_bytes': sum(len(str(msg)) for msg in getattr(self, 'chat_history', [])), |
There was a problem hiding this comment.
chat_history_size_bytes is not actually computing bytes (it sums character counts of str(msg)). Either rename the key to reflect what it measures (e.g., _chars/_repr_chars) or compute bytes explicitly (e.g., encoding to UTF-8) so the metric name matches the value.
| 'chat_history_size_bytes': sum(len(str(msg)) for msg in getattr(self, 'chat_history', [])), | |
| 'chat_history_size_bytes': sum(len(str(msg).encode("utf-8")) for msg in getattr(self, 'chat_history', [])), |
![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 refactors the Agent class by decomposing its functionality into new mixins: ChatMixin, ExecutionMixin, and MemoryMixin. Review feedback highlights critical thread-safety issues in the MemoryMixin regarding chat history management, which lacks the locking mechanisms present in the original implementation. Other improvements include deduplicating lazy-loading logic into a shared utility, ensuring consistent property naming for the stream_emitter, and correcting type hints to use typing.Callable.
| if not hasattr(self, 'chat_history'): | ||
| setattr(self, 'chat_history', []) | ||
|
|
||
| getattr(self, 'chat_history').append({ | ||
| "role": role, | ||
| "content": content | ||
| }) |
There was a problem hiding this comment.
The implementation of _add_to_chat_history in MemoryMixin is not thread-safe. The Agent class (line 1814 in agent.py) uses self._history_lock to protect modifications to chat_history. If this mixin is intended to eventually replace the logic in Agent, it must incorporate the same locking mechanism to prevent race conditions in multi-threaded environments.
| if not history or history[-1].get('content') != content or history[-1].get('role') != role: | ||
| self._add_to_chat_history(role, content) |
There was a problem hiding this comment.
The _add_to_chat_history_if_not_duplicate method lacks thread safety. In the Agent class (line 1824 in agent.py), this operation is performed atomically under a lock to prevent Time-of-Check to Time-of-Use (TOCTOU) race conditions where multiple threads might check for a duplicate and then both add the same message.
| if hasattr(self, 'chat_history'): | ||
| history = getattr(self, 'chat_history') | ||
| if len(history) > length: | ||
| setattr(self, 'chat_history', history[-length:]) |
There was a problem hiding this comment.
The _truncate_chat_history method is not thread-safe. It should use the agent's history lock when modifying the chat_history list to ensure consistency, matching the implementation found in agent.py (line 1852).
| def _get_llm_functions(): | ||
| """Lazy load LLM functions (thread-safe).""" | ||
| global _llm_module | ||
| if _llm_module is None: | ||
| with _lazy_import_lock: | ||
| if _llm_module is None: | ||
| from ..llm import get_openai_client, process_stream_chunks | ||
| _llm_module = { | ||
| 'get_openai_client': get_openai_client, | ||
| 'process_stream_chunks': process_stream_chunks, | ||
| } | ||
| return _llm_module | ||
|
|
||
| def _get_display_functions(): | ||
| """Lazy load display functions from main module (thread-safe).""" | ||
| global _main_module | ||
| if _main_module is None: | ||
| with _lazy_import_lock: | ||
| if _main_module is None: | ||
| from ..main import ( | ||
| display_error, | ||
| display_instruction, | ||
| display_interaction, | ||
| display_generating, | ||
| display_self_reflection, | ||
| ReflectionOutput, | ||
| adisplay_instruction, | ||
| execute_sync_callback | ||
| ) | ||
| _main_module = { | ||
| 'display_error': display_error, | ||
| 'display_instruction': display_instruction, | ||
| 'display_interaction': display_interaction, | ||
| 'display_generating': display_generating, | ||
| 'display_self_reflection': display_self_reflection, | ||
| 'ReflectionOutput': ReflectionOutput, | ||
| 'adisplay_instruction': adisplay_instruction, | ||
| 'execute_sync_callback': execute_sync_callback, | ||
| } | ||
| return _main_module |
There was a problem hiding this comment.
The lazy loading logic for LLM and display functions is duplicated from agent.py. This duplication increases maintenance overhead and potential for inconsistency. Consider moving these shared utilities to a common module (e.g., praisonaiagents/agent/utils.py) that both Agent and the mixins can import.
| @property | ||
| def stream_emitter(self) -> Optional[Any]: | ||
| """Get the current stream emitter.""" | ||
| return getattr(self, '_stream_emitter', None) | ||
|
|
||
| @stream_emitter.setter | ||
| def stream_emitter(self, value: Optional[Any]) -> None: | ||
| """Set the stream emitter.""" | ||
| setattr(self, '_stream_emitter', value) |
There was a problem hiding this comment.
The stream_emitter property in ChatMixin uses _stream_emitter as its internal attribute, while the Agent class (line 227 in agent.py) uses __stream_emitter and includes lazy initialization logic. This inconsistency will lead to issues when methods in ChatMixin attempt to use the emitter. Since Agent already implements this property with necessary lazy loading, it should be removed from the mixin or unified to use the same internal attribute and initialization pattern.
| raise NotImplementedError("Async start implementation needs to be moved from agent.py") | ||
|
|
||
| def run_until(self, max_iterations: int = 10, max_time: Optional[float] = None, | ||
| condition: Optional[callable] = None, **kwargs) -> Any: |
There was a problem hiding this comment.
The type hint for the condition parameter uses the built-in callable instead of Callable from the typing module. While callable works at runtime for checks, Callable is the standard for type annotations and allows specifying argument and return types.
| condition: Optional[callable] = None, **kwargs) -> Any: | |
| condition: Optional[Callable] = None, **kwargs) -> Any: |
| raise NotImplementedError("Run until implementation needs to be moved from agent.py") | ||
|
|
||
| async def run_until_async(self, max_iterations: int = 10, max_time: Optional[float] = None, | ||
| condition: Optional[callable] = None, **kwargs) -> Any: |
There was a problem hiding this comment.
The type hint for the condition parameter uses the built-in callable instead of Callable from the typing module.
| condition: Optional[callable] = None, **kwargs) -> Any: | |
| condition: Optional[Callable] = None, **kwargs) -> Any: |
![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 (4)
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py (2)
82-85: Replacesetattrwith direct attribute assignment.Using
setattrwith a constant attribute name is unnecessary and less readable than direct assignment.Suggested fix
`@stream_emitter.setter` def stream_emitter(self, value: Optional[Any]) -> None: """Set the stream emitter.""" - setattr(self, '_stream_emitter', value) + self._stream_emitter = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py` around lines 82 - 85, The setter for stream_emitter uses setattr(self, '_stream_emitter', value) which is unnecessary; replace it with direct assignment to the attribute (self._stream_emitter = value) inside the stream_emitter.setter to improve readability and simplicity while preserving behavior.
87-93: Consider consolidating parameters into Config objects.The method has 16 parameters. Per coding guidelines, agent parameters should be consolidated into Config objects following the
False=disabled, True=defaults, Config=custompattern. Since this is a new mixin establishing the framework, this is an opportunity to introduce cleaner APIs.Example groupings:
output_json,output_pydantic,reasoning_steps→OutputConfigforce_retrieval,skip_retrieval→ retrieval options in a configtask_name,task_description,task_id→TaskContextThis could be a follow-up refactor when moving the actual implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py` around lines 87 - 93, The chat method currently has too many parameters (chat in chat_mixin.py) — refactor to consolidate related parameters into config objects using the False=disabled, True=defaults, Config=custom pattern: create an OutputConfig to hold output_json, output_pydantic, reasoning_steps; a RetrievalConfig for force_retrieval and skip_retrieval; and a TaskContext for task_name, task_description, task_id; then change chat(prompt, temperature, tools, output: Union[bool, OutputConfig]=True, retrieval: Union[bool, RetrievalConfig]=False, task: Union[bool, TaskContext]=True, stream=None, config=None, attachments=None, tool_choice=None) (or equivalent names) and update internal usage in chat to read from these config objects or booleans accordingly so callers can pass False/True/or a config instance.src/praisonai-agents/praisonaiagents/agent/agent.py (1)
203-203: Avoid dual concrete ownership during decomposition.Line 203 wires
ChatMixin,ExecutionMixin, andMemoryMixinintoAgentwhileAgentstill carries overlapping concrete behavior (chat,_chat_impl,run,_cache_get, etc.). This creates two sources of truth during migration and raises drift risk. Prefer keeping mixins protocol/skeleton-only until each method is moved atomically.Based on learnings: "Implement DRY principle: reuse existing abstractions, refactor duplication safely, and check existing protocols before creating new ones instead of duplicating functionality" and "Use Protocol-driven core for praisonaiagents (no heavy implementations, only protocols/hooks/adapters/base classes) and move heavy implementations to the praisonai wrapper package".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/agent.py` at line 203, Agent currently mixes in ChatMixin, ExecutionMixin, and MemoryMixin while also retaining concrete implementations (e.g., chat, _chat_impl, run, _cache_get), creating dual ownership; refactor so mixins are protocol/abstract-only and move concrete logic out: convert ChatMixin, ExecutionMixin, MemoryMixin to abstract base classes or Protocols exposing method signatures (declare abstract methods like chat/_chat_impl/run/_cache_get), remove their concrete implementations from both the mixins and Agent, and place the full implementations into the praisonai wrapper package or a separate implementation class that Agent composes/delegates to; update Agent to call the abstract mixin methods (or delegate to the implementation class) so behavior is implemented in one place and the mixins remain skeletons.src/praisonai-agents/praisonaiagents/agent/execution_mixin.py (1)
237-256: Use thecontextpayload on the failure path.
_handle_execution_error()acceptscontextbut drops it completely. That loses the agent/session metadata and interception hook the SDK wants on execution failures.As per coding guidelines: "Error handling: Fail fast with clear error messages; include remediation hints in exceptions; propagate context (agent name, tool name, session ID); provide hook points for error interception (on_error events)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines 237 - 256, The _handle_execution_error method currently ignores the provided context; update it to log and propagate that context and to call any interception hook before re-raising: include agent/session metadata (use keys like session_id, tool_name, agent_name or context.get('tool')) in the logger message alongside self.name, invoke an on_error hook if present in context (e.g., context.get('on_error') or context.get('hooks', {}).get('on_error')) so external listeners can intercept, and when re-raising wrap the original exception with a new one that includes a remediation hint and the context payload (or attach context to the exception) instead of simply raising error so callers receive the metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py`:
- Around line 259-274: The _handle_tool_calls method currently returns an empty
list which silently drops tool-call results; make this stub consistent with
other unimplemented methods by raising NotImplementedError instead of returning
[] so callers can't accidentally ignore tool-call handling. Update the
_handle_tool_calls method in chat_mixin.py to raise NotImplementedError with a
brief message (e.g., "_handle_tool_calls not implemented") and keep the behavior
consistent with chat(), achat(), _build_multimodal_prompt(), and
chat_with_context(); leave intentional no-ops like
_format_response()/_process_agent_output() documented if they should remain as
pass-throughs.
- Around line 163-165: The async method achat has an inconsistent signature
versus its sync counterpart chat; add the missing parameters (config:
Optional[Dict[str, Any]], force_retrieval: bool, skip_retrieval: bool, stream:
Optional[bool], tool_choice: Optional[str]) to achat with the same types and
default values used by chat, update any internal calls/parameter forwarding
inside achat to pass these through to the shared implementation (or to whatever
helper chat uses), and ensure docstring and tests reflect the unified interface
so sync/async variants behave identically.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Around line 70-108: The public API is inverted: make start() the
async/long-running entry point and provide a sync run() wrapper that calls it;
specifically, change start (currently defined as def start) into an async def
start that implements the long-running behavior (or delegates to the existing
async logic now in astart), keep or remove astart as needed for backward
compatibility, and add a new run(self, prompt: Optional[str] = None, **kwargs)
sync method that calls asyncio.run(self.start(prompt, **kwargs)) or an
equivalent event-loop-safe runner; update references to ExecutionMixin.start,
ExecutionMixin.astart, and the new ExecutionMixin.run so callers use run() for
synchronous entry and start() for async.
- Around line 29-40: Remove logging of raw prompt/goal text in the run method
and other logging sites (e.g., the logger.debug calls in run, any
_execute/_handle methods at the ranges noted) and replace them with
non-sensitive metadata such as the prompt/goal length, a hash or redacted
snippet, and the agent/mode names; specifically update the logger.debug in run
(and the similar debug calls at lines ~52-63, ~70-82, ~92-103, ~156-170) to
report only safe info like f"{self.name}: Running with prompt
length={len(prompt)} and kwargs keys={list(kwargs.keys())}" or a short masked
summary instead of the raw payload so no user content is emitted to logs.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py`:
- Around line 133-136: The truncation logic in _truncate_chat_history (the block
that checks hasattr(self, 'chat_history') and does setattr(self, 'chat_history',
history[-length:])) fails when length is 0 because history[-0:] returns the full
list; fix it by explicitly handling length == 0 (or length <= 0) — set
chat_history to an empty list when length is 0, otherwise use history[-length:];
apply the same check to the other call sites mentioned around the
max_chat_history adjustment (lines referencing max_chat_history and the call to
_truncate_chat_history).
- Around line 70-79: The _add_to_chat_history method currently logs message
content via logger.debug(f"{self.name}: Adding {role} message to chat history:
{content[:100]}..."), which can leak secrets/PII; change the logger.debug call
in memory_mixin.py to only record non-sensitive metadata such as self.name,
role, and content length (e.g., len(content)) or a boolean flag for emptiness,
and remove any substring or full content logging; update the log message invoked
in _add_to_chat_history to reflect only role/size metadata and retain the rest
of the method unchanged.
- Around line 153-157: The current returns and persistence use shallow list
copies (history.copy()) and pass the same list objects in
_save_memory_state()/_load_memory_state(), allowing external mutation of
agent-owned message dicts; replace those shallow copies by creating and
returning deep snapshots (e.g., deep-copy each message dict) wherever
chat_history is exposed (the branch in the method that checks include_system,
and the similar code at the spots you noted around lines 181-184 and 202-203),
and ensure _save_memory_state() and _load_memory_state() store and return
deep-copied lists of message dicts so the agent always owns immutable snapshots
rather than live objects (reference symbols: chat_history, include_system
branch, _save_memory_state, _load_memory_state).
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agent/agent.py`:
- Line 203: Agent currently mixes in ChatMixin, ExecutionMixin, and MemoryMixin
while also retaining concrete implementations (e.g., chat, _chat_impl, run,
_cache_get), creating dual ownership; refactor so mixins are
protocol/abstract-only and move concrete logic out: convert ChatMixin,
ExecutionMixin, MemoryMixin to abstract base classes or Protocols exposing
method signatures (declare abstract methods like
chat/_chat_impl/run/_cache_get), remove their concrete implementations from both
the mixins and Agent, and place the full implementations into the praisonai
wrapper package or a separate implementation class that Agent composes/delegates
to; update Agent to call the abstract mixin methods (or delegate to the
implementation class) so behavior is implemented in one place and the mixins
remain skeletons.
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py`:
- Around line 82-85: The setter for stream_emitter uses setattr(self,
'_stream_emitter', value) which is unnecessary; replace it with direct
assignment to the attribute (self._stream_emitter = value) inside the
stream_emitter.setter to improve readability and simplicity while preserving
behavior.
- Around line 87-93: The chat method currently has too many parameters (chat in
chat_mixin.py) — refactor to consolidate related parameters into config objects
using the False=disabled, True=defaults, Config=custom pattern: create an
OutputConfig to hold output_json, output_pydantic, reasoning_steps; a
RetrievalConfig for force_retrieval and skip_retrieval; and a TaskContext for
task_name, task_description, task_id; then change chat(prompt, temperature,
tools, output: Union[bool, OutputConfig]=True, retrieval: Union[bool,
RetrievalConfig]=False, task: Union[bool, TaskContext]=True, stream=None,
config=None, attachments=None, tool_choice=None) (or equivalent names) and
update internal usage in chat to read from these config objects or booleans
accordingly so callers can pass False/True/or a config instance.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Around line 237-256: The _handle_execution_error method currently ignores the
provided context; update it to log and propagate that context and to call any
interception hook before re-raising: include agent/session metadata (use keys
like session_id, tool_name, agent_name or context.get('tool')) in the logger
message alongside self.name, invoke an on_error hook if present in context
(e.g., context.get('on_error') or context.get('hooks', {}).get('on_error')) so
external listeners can intercept, and when re-raising wrap the original
exception with a new one that includes a remediation hint and the context
payload (or attach context to the exception) instead of simply raising error so
callers receive the metadata.
🪄 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: 4b855227-0429-4e3f-9851-249bfe213410
📒 Files selected for processing (4)
src/praisonai-agents/praisonaiagents/agent/agent.pysrc/praisonai-agents/praisonaiagents/agent/chat_mixin.pysrc/praisonai-agents/praisonaiagents/agent/execution_mixin.pysrc/praisonai-agents/praisonaiagents/agent/memory_mixin.py
| async def achat(self, prompt: str, temperature=1.0, tools=None, output_json=None, | ||
| output_pydantic=None, reasoning_steps=False, task_name=None, | ||
| task_description=None, task_id=None, attachments=None): |
There was a problem hiding this comment.
Parameter signature inconsistency between chat() and achat().
achat() is missing several parameters that exist in chat():
config: Optional[Dict[str, Any]]force_retrieval: boolskip_retrieval: boolstream: Optional[bool]tool_choice: Optional[str]
Per coding guidelines, sync and async variants should have matching interfaces. This inconsistency will cause issues when users switch between sync and async calls.
Suggested signature alignment
- async def achat(self, prompt: str, temperature=1.0, tools=None, output_json=None,
- output_pydantic=None, reasoning_steps=False, task_name=None,
- task_description=None, task_id=None, attachments=None):
+ async def achat(self, prompt: str, temperature: float = 1.0, tools: Optional[List[Any]] = None,
+ output_json: Optional[Any] = None, output_pydantic: Optional[Any] = None,
+ reasoning_steps: bool = False, stream: Optional[bool] = None,
+ task_name: Optional[str] = None, task_description: Optional[str] = None,
+ task_id: Optional[str] = None, config: Optional[Dict[str, Any]] = None,
+ force_retrieval: bool = False, skip_retrieval: bool = False,
+ attachments: Optional[List[str]] = None, tool_choice: Optional[str] = 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.
| async def achat(self, prompt: str, temperature=1.0, tools=None, output_json=None, | |
| output_pydantic=None, reasoning_steps=False, task_name=None, | |
| task_description=None, task_id=None, attachments=None): | |
| async def achat(self, prompt: str, temperature: float = 1.0, tools: Optional[List[Any]] = None, | |
| output_json: Optional[Any] = None, output_pydantic: Optional[Any] = None, | |
| reasoning_steps: bool = False, stream: Optional[bool] = None, | |
| task_name: Optional[str] = None, task_description: Optional[str] = None, | |
| task_id: Optional[str] = None, config: Optional[Dict[str, Any]] = None, | |
| force_retrieval: bool = False, skip_retrieval: bool = False, | |
| attachments: Optional[List[str]] = None, tool_choice: Optional[str] = None): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py` around lines 163 -
165, The async method achat has an inconsistent signature versus its sync
counterpart chat; add the missing parameters (config: Optional[Dict[str, Any]],
force_retrieval: bool, skip_retrieval: bool, stream: Optional[bool],
tool_choice: Optional[str]) to achat with the same types and default values used
by chat, update any internal calls/parameter forwarding inside achat to pass
these through to the shared implementation (or to whatever helper chat uses),
and ensure docstring and tests reflect the unified interface so sync/async
variants behave identically.
| def _handle_tool_calls(self, tool_calls: List[Dict[str, Any]], **kwargs) -> List[Any]: | ||
| """ | ||
| Handle tool calls in the context of chat interactions. | ||
|
|
||
| Args: | ||
| tool_calls: List of tool call objects from LLM | ||
| **kwargs: Additional handling parameters | ||
|
|
||
| Returns: | ||
| List of tool call results | ||
| """ | ||
| # TODO: Move tool call handling logic from agent.py | ||
| # This includes tool execution, result formatting, and error handling | ||
|
|
||
| logger.debug(f"{self.name}: Handling {len(tool_calls)} tool calls") | ||
| return [] |
There was a problem hiding this comment.
Inconsistent stub behavior: silent pass-through vs NotImplementedError.
_handle_tool_calls returns an empty list instead of raising NotImplementedError like other placeholder methods. This creates inconsistent behavior:
chat(),achat(),_build_multimodal_prompt(),chat_with_context()→ raiseNotImplementedError_format_response(),_process_agent_output(),_handle_tool_calls()→ silent pass-through
Returning [] for tool calls means tool results are silently dropped, which could cause subtle bugs if this method is inadvertently used before the actual implementation is migrated.
Consider making all stubs consistent—either all raise NotImplementedError or clearly document which methods are intentional no-ops.
Option: Make stubs consistent
def _handle_tool_calls(self, tool_calls: List[Dict[str, Any]], **kwargs) -> List[Any]:
# TODO: Move tool call handling logic from agent.py
# This includes tool execution, result formatting, and error handling
logger.debug(f"{self.name}: Handling {len(tool_calls)} tool calls")
- return []
+ raise NotImplementedError("Tool call handling needs to be moved from agent.py")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py` around lines 259 -
274, The _handle_tool_calls method currently returns an empty list which
silently drops tool-call results; make this stub consistent with other
unimplemented methods by raising NotImplementedError instead of returning [] so
callers can't accidentally ignore tool-call handling. Update the
_handle_tool_calls method in chat_mixin.py to raise NotImplementedError with a
brief message (e.g., "_handle_tool_calls not implemented") and keep the behavior
consistent with chat(), achat(), _build_multimodal_prompt(), and
chat_with_context(); leave intentional no-ops like
_format_response()/_process_agent_output() documented if they should remain as
pass-throughs.
| def run(self, prompt: str, **kwargs: Any) -> Optional[str]: | ||
| """ | ||
| Run the agent with a prompt synchronously. | ||
|
|
||
| Args: | ||
| prompt: The input prompt/task for the agent | ||
| **kwargs: Additional arguments passed to the execution | ||
|
|
||
| Returns: | ||
| The agent's response or None if execution fails | ||
| """ | ||
| logger.debug(f"{self.name}: Running with prompt: {prompt[:100]}...") |
There was a problem hiding this comment.
Avoid logging raw prompt and goal text.
These debug statements emit user prompt/goal content directly. That data is often sensitive, so log lengths/modes instead of the payload itself.
Also applies to: 52-63, 70-82, 92-103, 156-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines
29 - 40, Remove logging of raw prompt/goal text in the run method and other
logging sites (e.g., the logger.debug calls in run, any _execute/_handle methods
at the ranges noted) and replace them with non-sensitive metadata such as the
prompt/goal length, a hash or redacted snippet, and the agent/mode names;
specifically update the logger.debug in run (and the similar debug calls at
lines ~52-63, ~70-82, ~92-103, ~156-170) to report only safe info like
f"{self.name}: Running with prompt length={len(prompt)} and kwargs
keys={list(kwargs.keys())}" or a short masked summary instead of the raw payload
so no user content is emitted to logs.
| def start(self, prompt: Optional[str] = None, **kwargs: Any) -> Union[str, Generator[str, None, None], None]: | ||
| """ | ||
| Start the agent with optional prompt (supports both single execution and continuous running). | ||
|
|
||
| Args: | ||
| prompt: Optional input prompt. If None, may start in interactive mode | ||
| **kwargs: Additional startup arguments | ||
|
|
||
| Returns: | ||
| Either a string response, a generator for streaming, or None | ||
| """ | ||
| logger.debug(f"{self.name}: Starting with prompt: {prompt[:100] if prompt else 'None'}...") | ||
|
|
||
| # TODO: Move actual implementation from agent.py lines 7603+ | ||
| # This includes: | ||
| # - Startup initialization | ||
| # - Mode detection (single vs continuous) | ||
| # - Streaming support | ||
| # - Interactive mode handling | ||
|
|
||
| raise NotImplementedError("Start implementation needs to be moved from agent.py") | ||
|
|
||
| async def astart(self, prompt: str, **kwargs): | ||
| """ | ||
| Start the agent asynchronously. | ||
|
|
||
| Args: | ||
| prompt: Input prompt for the agent | ||
| **kwargs: Additional startup arguments | ||
|
|
||
| Returns: | ||
| Agent's response | ||
| """ | ||
| logger.debug(f"{self.name}: Async starting with prompt: {prompt[:100]}...") | ||
|
|
||
| # TODO: Move actual implementation from agent.py lines 7339+ | ||
| # Should be async version of start() | ||
|
|
||
| raise NotImplementedError("Async start implementation needs to be moved from agent.py") |
There was a problem hiding this comment.
Align start() with the repo's async contract.
start() is synchronous here while astart() carries the async variant. In this codebase, run() is the sync entry point and start() is the async/long-running one, so moving this shape into the mixin bakes in the opposite public API.
Based on learnings: "Implement both sync and async entry points for user-facing APIs (run() for sync, start() for async); internal APIs should prefer async"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines
70 - 108, The public API is inverted: make start() the async/long-running entry
point and provide a sync run() wrapper that calls it; specifically, change start
(currently defined as def start) into an async def start that implements the
long-running behavior (or delegates to the existing async logic now in astart),
keep or remove astart as needed for backward compatibility, and add a new
run(self, prompt: Optional[str] = None, **kwargs) sync method that calls
asyncio.run(self.start(prompt, **kwargs)) or an equivalent event-loop-safe
runner; update references to ExecutionMixin.start, ExecutionMixin.astart, and
the new ExecutionMixin.run so callers use run() for synchronous entry and
start() for async.
| def _add_to_chat_history(self, role: str, content: str) -> None: | ||
| """ | ||
| Add a message to the chat history. | ||
|
|
||
| Args: | ||
| role: Role of the message sender ('user', 'assistant', 'system') | ||
| content: Message content | ||
| """ | ||
| logger.debug(f"{self.name}: Adding {role} message to chat history: {content[:100]}...") | ||
|
|
There was a problem hiding this comment.
Don't log chat payload content.
Line 78 writes the first 100 characters of every message into debug logs. Prompts and responses routinely contain secrets or PII, so this should log role/size metadata only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py` around lines 70 -
79, The _add_to_chat_history method currently logs message content via
logger.debug(f"{self.name}: Adding {role} message to chat history:
{content[:100]}..."), which can leak secrets/PII; change the logger.debug call
in memory_mixin.py to only record non-sensitive metadata such as self.name,
role, and content length (e.g., len(content)) or a boolean flag for emptiness,
and remove any substring or full content logging; update the log message invoked
in _add_to_chat_history to reflect only role/size metadata and retain the rest
of the method unchanged.
| if hasattr(self, 'chat_history'): | ||
| history = getattr(self, 'chat_history') | ||
| if len(history) > length: | ||
| setattr(self, 'chat_history', history[-length:]) |
There was a problem hiding this comment.
Small max_chat_history values are never enforced.
When max_chat_history is 1, Line 268 passes 0 into _truncate_chat_history(), and Line 136 evaluates history[-0:], which returns the full list. That silently ignores the configured retention cap.
Proposed fix
def _truncate_chat_history(self, length: int) -> None:
...
if hasattr(self, 'chat_history'):
history = getattr(self, 'chat_history')
+ if length <= 0:
+ setattr(self, 'chat_history', [])
+ return
if len(history) > length:
setattr(self, 'chat_history', history[-length:])
...
max_history = getattr(self, 'max_chat_history', 1000)
if hasattr(self, 'chat_history') and len(getattr(self, 'chat_history')) > max_history:
- self._truncate_chat_history(max_history // 2)
+ self._truncate_chat_history(max_history)Also applies to: 265-268
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 134-134: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 136-136: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py` around lines 133
- 136, The truncation logic in _truncate_chat_history (the block that checks
hasattr(self, 'chat_history') and does setattr(self, 'chat_history',
history[-length:])) fails when length is 0 because history[-0:] returns the full
list; fix it by explicitly handling length == 0 (or length <= 0) — set
chat_history to an empty list when length is 0, otherwise use history[-length:];
apply the same check to the other call sites mentioned around the
max_chat_history adjustment (lines referencing max_chat_history and the call to
_truncate_chat_history).
| history = getattr(self, 'chat_history') | ||
| if include_system: | ||
| return history.copy() | ||
| else: | ||
| return [msg for msg in history if msg.get('role') != 'system'] |
There was a problem hiding this comment.
Return and persist snapshots, not live chat-history objects.
history.copy() only clones the list, so callers can still mutate the message dicts in place. _save_memory_state() and _load_memory_state() also pass the same list object across the boundary, which lets outside code keep mutating agent-owned memory after save/load.
Based on learnings: "Maintain multi-agent safety: avoid shared mutable global state between agents, ensure each agent owns its context/memory/session, and use explicit channels (EventBus, handoff) for cross-agent communication"
Also applies to: 181-184, 202-203
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 153-153: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py` around lines 153
- 157, The current returns and persistence use shallow list copies
(history.copy()) and pass the same list objects in
_save_memory_state()/_load_memory_state(), allowing external mutation of
agent-owned message dicts; replace those shallow copies by creating and
returning deep snapshots (e.g., deep-copy each message dict) wherever
chat_history is exposed (the branch in the method that checks include_system,
and the similar code at the spots you noted around lines 181-184 and 202-203),
and ensure _save_memory_state() and _load_memory_state() store and return
deep-copied lists of message dicts so the agent always owns immutable snapshots
rather than live objects (reference symbols: chat_history, include_system
branch, _save_memory_state, _load_memory_state).
|
Closing in favor of unified tracking issue #1254 to prevent duplicate efforts. |
|
Claude finished @MervinPraison's task —— View job • ✅ Completed: Code review feedback addressedSuccessfully addressed the code review feedback from PR #1243 and created a new PR with focused fixes. Todo List:
✅ Changes Made:
✅ Testing Results:🚀 New PR Created:PR #1256 - Addresses all reviewer feedback while maintaining backward compatibility Architecture Notes:Per AGENTS.md guidelines, the core SDK should be protocol-driven with lightweight interfaces. The current mixin approach establishes the framework foundation. Future improvements should consider:
This provides a solid foundation for the god class decomposition while following the repository's architectural principles. |
This establishes the mixin framework for decomposing agent.py from 8,915 lines to ≤5000 lines. Actual method implementations need to be moved in future work.
Summary by CodeRabbit