fix: resolve sync/async path drift, silent DB writes, and adapter inconsistency#1884
fix: resolve sync/async path drift, silent DB writes, and adapter inconsistency#1884praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…onsistency (fixes #1883) - Extract shared pipeline logic in AgentsGenerator to eliminate sync/async drift - Add _init_stores() to all DB write hooks to prevent silent data loss - Add _resolve_llm() to BaseFrameworkAdapter for consistent string/dict llm handling - Fix CrewAI adapter to support string-form llm configuration like PraisonAI adapter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAgentsGenerator consolidates duplicated sync/async preparation into shared ChangesExecution Path Consolidation and LLM Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR refactors three independent wrapper-layer fixes: it extracts duplicated sync/async preparation logic in
Confidence Score: 4/5The DB and CrewAI adapter fixes are safe to merge. The agents_generator refactor is correct on the sync path but introduces new synchronous work (observability init and adapter setup) into the async execution path that warrants verification before merging. The src/praisonai/praisonai/agents_generator.py — specifically Important Files Changed
Reviews (2): Last reviewed commit: "fix: address IndexError, dead code, and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai/praisonai/db/adapter.py (1)
446-524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
_init_stores()to tracing handlers to prevent silent data loss.The tracing handlers (
on_trace_start,on_trace_end,on_span_start,on_span_end) checkself._state_storewithout initializing stores first. Sinceself._state_storeisNoneuntil_init_stores()is called (Line 62), traces and spans will be silently dropped if these handlers are invoked before any read operation (e.g., beforeon_agent_start).This is the same bug pattern that this PR fixes for message and run handlers.
🛡️ Proposed fix to add store initialization
def on_trace_start( self, trace_id: str, session_id: Optional[str] = None, run_id: Optional[str] = None, agent_name: Optional[str] = None, user_id: Optional[str] = None, metadata: Optional[Dict[str, Any]] = None, ) -> None: """Called when a new trace starts.""" + self._init_stores() if self._state_store: trace_key = f"trace:{trace_id}" self._state_store.set(trace_key, { "trace_id": trace_id, "session_id": session_id, "run_id": run_id, "agent_name": agent_name, "user_id": user_id, "started_at": time.time(), "status": "running", "spans": [], "metadata": metadata or {} }) def on_trace_end( self, trace_id: str, status: str = "ok", metadata: Optional[Dict[str, Any]] = None, ) -> None: """Called when a trace ends.""" + self._init_stores() if self._state_store: trace_key = f"trace:{trace_id}" trace_data = self._state_store.get(trace_key) or {} trace_data.update({ "ended_at": time.time(), "status": status, "metadata": {**trace_data.get("metadata", {}), **(metadata or {})} }) self._state_store.set(trace_key, trace_data) def on_span_start( self, span_id: str, trace_id: str, name: str, parent_span_id: Optional[str] = None, attributes: Optional[Dict[str, Any]] = None, ) -> None: """Called when a new span starts.""" + self._init_stores() if self._state_store: span_key = f"span:{span_id}" self._state_store.set(span_key, { "span_id": span_id, "trace_id": trace_id, "name": name, "parent_span_id": parent_span_id, "started_at": time.time(), "status": "running", "attributes": attributes or {}, "events": [] }) def on_span_end( self, span_id: str, status: str = "ok", attributes: Optional[Dict[str, Any]] = None, ) -> None: """Called when a span ends.""" + self._init_stores() if self._state_store: span_key = f"span:{span_id}" span_data = self._state_store.get(span_key) or {} span_data.update({ "ended_at": time.time(), "status": status, "attributes": {**span_data.get("attributes", {}), **(attributes or {})} }) self._state_store.set(span_key, span_data)🤖 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/db/adapter.py` around lines 446 - 524, The trace/span handlers (on_trace_start, on_trace_end, on_span_start, on_span_end) read self._state_store without ensuring stores are initialized, causing silent drops if _init_stores() hasn't run; fix by calling self._init_stores() at the start of each of these four methods (before checking or using self._state_store) so the state store is created if needed, then proceed with the existing logic that reads/writes trace and span keys.
🧹 Nitpick comments (4)
src/praisonai/praisonai/db/adapter.py (1)
191-336: ⚖️ Poor tradeoffConsider using a decorator to enforce store initialization.
The PR objectives mention preferring a decorator approach (e.g.,
@_ensures_init) to enforce_init_stores()on write hooks. While the current explicit calls work correctly, a decorator would:
- Reduce code duplication across handlers
- Prevent accidentally omitting initialization in future write handlers
- Make the initialization contract more explicit and maintainable
♻️ Example decorator implementation
from functools import wraps def _ensures_init(func): """Decorator to ensure stores are initialized before handler execution.""" `@wraps`(func) def wrapper(self, *args, **kwargs): self._init_stores() return func(self, *args, **kwargs) return wrapperThen apply to handlers:
`@_ensures_init` def on_user_message(self, session_id: str, content: str, ...): if not self._conversation_store: return # ... rest of implementation🤖 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/db/adapter.py` around lines 191 - 336, The handlers repeat self._init_stores() and risk future omissions; create a decorator named _ensures_init that calls self._init_stores() before running the wrapped method, then apply it to the write handlers (on_user_message, on_agent_message, on_tool_call, on_agent_end, on_run_start, on_run_end) and remove the explicit self._init_stores() calls from their bodies; keep the existing early-return checks for self._conversation_store/_state_store in each handler so behavior is unchanged.src/praisonai/praisonai/agents_generator.py (2)
475-506: ⚖️ Poor tradeoffAdapter instantiation for availability checks is inefficient.
Lines 478-479 create adapter instances via
_get_framework_adapter()solely foris_available()checks. If adapter construction is heavyweight, this wastes resources. Consider:
- Caching adapters after first instantiation, or
- Adding a static/class method for availability checks that doesn't require instance creation.
That said, if adapters are lightweight, this is acceptable for the once-per-run execution context.
🤖 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/agents_generator.py` around lines 475 - 506, The current _select_autogen_version calls _get_framework_adapter("autogen_v4") and _get_framework_adapter("autogen") only to call is_available(), which can be inefficient if adapter construction is heavy; update the approach by either (A) caching adapter instances on the object (e.g., store results from _get_framework_adapter in a dict/attributes like self._adapter_cache and reuse autogen_v4_adapter/autogen_v2_adapter on subsequent calls) or (B) add a class/static availability check (e.g., FrameworkAdapter.is_available_static("autogen_v4")) so _select_autogen_version can check availability without instantiating full adapters; modify _select_autogen_version to use the chosen strategy and ensure _get_framework_adapter and any new cache or static method are implemented and used consistently, keeping references to autogen_v4_adapter, autogen_v2_adapter, and is_available for locating the affected code.
346-410: ⚖️ Poor tradeoffSide effects in a method named "prepare" reduce predictability.
_prepare_for_run()performs multiple side effects:
- Mutates input
configdict (line 354): Adds'roles'section in-place, modifying the caller's dictionary.- Mutates instance state (lines 402-403): Updates
self.frameworkandself.framework_adapter.These side effects make the method harder to test and reason about, since "prepare" suggests a pure transformation. Consider either:
- Renaming to
_initialize_run()or_setup_run()to signal mutations, or- Making the method pure by returning the modified config and letting the caller update instance state.
🤖 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/agents_generator.py` around lines 346 - 410, _prepare_for_run currently mutates the input config (adds config['roles']) and instance state (self.framework, self.framework_adapter), which reduces predictability; pick one of two fixes: 1) rename the method to _initialize_run or _setup_run to signal side effects (update all callers) OR 2) make it pure by deep-copying config at the top (so you never modify the caller's dict), avoid setting self.framework/self.framework_adapter inside the function, and instead return the normalized config plus the resolved framework and adapter (e.g., include 'framework_name' and 'framework_adapter' in the returned dict) so the caller can assign self.framework and self.framework_adapter explicitly after calling _prepare_for_run. Ensure references to _validate_agents_config, _build_tools_dict, _select_autogen_version, _get_framework_adapter, init_observability, and assert_framework_available remain unchanged but operate on the copied/returned config and adapter.src/praisonai/praisonai/framework_adapters/base.py (1)
131-131: DEFAULT_MODEL format mismatch is compatible; only_resolve_llmcleanup needed
- The difference between
BaseFrameworkAdapter.DEFAULT_MODEL = "openai/gpt-4o-mini"andllm/env.py’s_DEFAULT_MODEL = "gpt-4o-mini"is intentional/supported:PraisonAIModelstrips an"openai/"prefix and derivesmodel_name/base_url, so both formats work._resolve_llm()’sfieldparameter is unused; remove it or use it.🤖 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/framework_adapters/base.py` at line 131, BaseFrameworkAdapter.DEFAULT_MODEL value is fine; the real issue is that the _resolve_llm function declares an unused parameter named field — either remove that parameter from _resolve_llm's signature and update every call site (e.g., where _resolve_llm(...) is invoked) or use the field value inside _resolve_llm to pick/validate which config field to resolve (for example, consult field to decide between model vs fine_tune settings) and update PraisonAIModel and related callers accordingly; ensure references to _resolve_llm, DEFAULT_MODEL, and PraisonAIModel remain consistent after the change.
🤖 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/framework_adapters/base.py`:
- Around line 148-164: The _resolve_llm method risks IndexError by reading
llm_config[0] without ensuring llm_config is a non-empty list and also declares
an unused parameter field; update _resolve_llm to safely extract base_url and
api_key by checking llm_config truthiness and length (e.g., if llm_config and
len(llm_config) > 0: base = llm_config[0].get('base_url'); key =
llm_config[0].get('api_key') else: base = key = None), and remove the unused
field parameter from the signature and any callers if present; keep the rest of
the logic (resolving spec to model and returning
PraisonAIModel(...).get_model()) unchanged.
---
Outside diff comments:
In `@src/praisonai/praisonai/db/adapter.py`:
- Around line 446-524: The trace/span handlers (on_trace_start, on_trace_end,
on_span_start, on_span_end) read self._state_store without ensuring stores are
initialized, causing silent drops if _init_stores() hasn't run; fix by calling
self._init_stores() at the start of each of these four methods (before checking
or using self._state_store) so the state store is created if needed, then
proceed with the existing logic that reads/writes trace and span keys.
---
Nitpick comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Around line 475-506: The current _select_autogen_version calls
_get_framework_adapter("autogen_v4") and _get_framework_adapter("autogen") only
to call is_available(), which can be inefficient if adapter construction is
heavy; update the approach by either (A) caching adapter instances on the object
(e.g., store results from _get_framework_adapter in a dict/attributes like
self._adapter_cache and reuse autogen_v4_adapter/autogen_v2_adapter on
subsequent calls) or (B) add a class/static availability check (e.g.,
FrameworkAdapter.is_available_static("autogen_v4")) so _select_autogen_version
can check availability without instantiating full adapters; modify
_select_autogen_version to use the chosen strategy and ensure
_get_framework_adapter and any new cache or static method are implemented and
used consistently, keeping references to autogen_v4_adapter, autogen_v2_adapter,
and is_available for locating the affected code.
- Around line 346-410: _prepare_for_run currently mutates the input config (adds
config['roles']) and instance state (self.framework, self.framework_adapter),
which reduces predictability; pick one of two fixes: 1) rename the method to
_initialize_run or _setup_run to signal side effects (update all callers) OR 2)
make it pure by deep-copying config at the top (so you never modify the caller's
dict), avoid setting self.framework/self.framework_adapter inside the function,
and instead return the normalized config plus the resolved framework and adapter
(e.g., include 'framework_name' and 'framework_adapter' in the returned dict) so
the caller can assign self.framework and self.framework_adapter explicitly after
calling _prepare_for_run. Ensure references to _validate_agents_config,
_build_tools_dict, _select_autogen_version, _get_framework_adapter,
init_observability, and assert_framework_available remain unchanged but operate
on the copied/returned config and adapter.
In `@src/praisonai/praisonai/db/adapter.py`:
- Around line 191-336: The handlers repeat self._init_stores() and risk future
omissions; create a decorator named _ensures_init that calls self._init_stores()
before running the wrapped method, then apply it to the write handlers
(on_user_message, on_agent_message, on_tool_call, on_agent_end, on_run_start,
on_run_end) and remove the explicit self._init_stores() calls from their bodies;
keep the existing early-return checks for self._conversation_store/_state_store
in each handler so behavior is unchanged.
In `@src/praisonai/praisonai/framework_adapters/base.py`:
- Line 131: BaseFrameworkAdapter.DEFAULT_MODEL value is fine; the real issue is
that the _resolve_llm function declares an unused parameter named field — either
remove that parameter from _resolve_llm's signature and update every call site
(e.g., where _resolve_llm(...) is invoked) or use the field value inside
_resolve_llm to pick/validate which config field to resolve (for example,
consult field to decide between model vs fine_tune settings) and update
PraisonAIModel and related callers accordingly; ensure references to
_resolve_llm, DEFAULT_MODEL, and PraisonAIModel remain consistent after the
change.
🪄 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: dac20688-d03a-4cbc-bf46-3bb52b464b1e
📒 Files selected for processing (4)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/db/adapter.pysrc/praisonai/praisonai/framework_adapters/base.pysrc/praisonai/praisonai/framework_adapters/crewai_adapter.py
| def _resolve_llm(self, spec, llm_config, *, field="llm"): | ||
| """Build a PraisonAIModel from a per-agent llm/function_calling_llm spec. | ||
| Accepts str, dict, or None. Single source of truth for all adapters.""" | ||
| from ..inc import PraisonAIModel | ||
| import os | ||
|
|
||
| base = llm_config[0].get('base_url') if llm_config else None | ||
| key = llm_config[0].get('api_key') if llm_config else None | ||
|
|
||
| if isinstance(spec, str) and spec.strip(): | ||
| model = spec.strip() | ||
| elif isinstance(spec, dict) and spec.get('model'): | ||
| model = spec['model'] | ||
| else: | ||
| model = os.environ.get("MODEL_NAME") or self.DEFAULT_MODEL | ||
|
|
||
| return PraisonAIModel(model=model, base_url=base, api_key=key).get_model() |
There was a problem hiding this comment.
Guard against IndexError and remove unused parameter.
Two issues:
- IndexError risk: Line 154-155 access
llm_config[0]without checking ifllm_configis non-empty. If an empty list is passed, this raisesIndexError. - Dead parameter: The
fieldparameter is defined but never used in the method body.
🛡️ Proposed fix
- def _resolve_llm(self, spec, llm_config, *, field="llm"):
+ def _resolve_llm(self, spec, llm_config):
"""Build a PraisonAIModel from a per-agent llm/function_calling_llm spec.
Accepts str, dict, or None. Single source of truth for all adapters."""
from ..inc import PraisonAIModel
import os
- base = llm_config[0].get('base_url') if llm_config else None
- key = llm_config[0].get('api_key') if llm_config else None
+ base = llm_config[0].get('base_url') if (llm_config and len(llm_config) > 0) else None
+ key = llm_config[0].get('api_key') if (llm_config and len(llm_config) > 0) else None
if isinstance(spec, str) and spec.strip():
model = spec.strip()📝 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.
| def _resolve_llm(self, spec, llm_config, *, field="llm"): | |
| """Build a PraisonAIModel from a per-agent llm/function_calling_llm spec. | |
| Accepts str, dict, or None. Single source of truth for all adapters.""" | |
| from ..inc import PraisonAIModel | |
| import os | |
| base = llm_config[0].get('base_url') if llm_config else None | |
| key = llm_config[0].get('api_key') if llm_config else None | |
| if isinstance(spec, str) and spec.strip(): | |
| model = spec.strip() | |
| elif isinstance(spec, dict) and spec.get('model'): | |
| model = spec['model'] | |
| else: | |
| model = os.environ.get("MODEL_NAME") or self.DEFAULT_MODEL | |
| return PraisonAIModel(model=model, base_url=base, api_key=key).get_model() | |
| def _resolve_llm(self, spec, llm_config): | |
| """Build a PraisonAIModel from a per-agent llm/function_calling_llm spec. | |
| Accepts str, dict, or None. Single source of truth for all adapters.""" | |
| from ..inc import PraisonAIModel | |
| import os | |
| base = llm_config[0].get('base_url') if (llm_config and len(llm_config) > 0) else None | |
| key = llm_config[0].get('api_key') if (llm_config and len(llm_config) > 0) else None | |
| if isinstance(spec, str) and spec.strip(): | |
| model = spec.strip() | |
| elif isinstance(spec, dict) and spec.get('model'): | |
| model = spec['model'] | |
| else: | |
| model = os.environ.get("MODEL_NAME") or self.DEFAULT_MODEL | |
| return PraisonAIModel(model=model, base_url=base, api_key=key).get_model() |
🤖 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/framework_adapters/base.py` around lines 148 - 164,
The _resolve_llm method risks IndexError by reading llm_config[0] without
ensuring llm_config is a non-empty list and also declares an unused parameter
field; update _resolve_llm to safely extract base_url and api_key by checking
llm_config truthiness and length (e.g., if llm_config and len(llm_config) > 0:
base = llm_config[0].get('base_url'); key = llm_config[0].get('api_key') else:
base = key = None), and remove the unused field parameter from the signature and
any callers if present; keep the rest of the logic (resolving spec to model and
returning PraisonAIModel(...).get_model()) unchanged.
|
@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 & Issue Resolution ✅Phase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
Architecture Analysis ✅Protocol-driven Design: ✅ COMPLIANT
Backward Compatibility: ✅ MAINTAINED
Performance: ✅ NO REGRESSIONS
Issues Fixed ✅Fixed 4 critical issues identified by reviewers:
Skipped non-critical suggestions:
Final Verdict: ✅ APPROVEThis PR successfully addresses the 3 high-impact wrapper layer issues:
Quality assurance:
Recommendation: This PR is ready for merge. The fixes are minimal, focused, and address real bugs without introducing regressions. Files modified: |
- Fix IndexError risk in BaseFrameworkAdapter._resolve_llm() by checking llm_config length - Remove unused 'field' parameter from _resolve_llm() method signature - Fix duplicate arun() method definition in BaseFrameworkAdapter - Add missing _init_stores() calls to all tracing handlers (on_trace_start, on_trace_end, on_span_start, on_span_end) Addresses feedback from CodeRabbit and Greptile reviews. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #1883
This PR addresses the 3 highest-impact wrapper layer issues identified in the issue analysis:
1. Sync/async path drift in agents_generator.py
2. Silent DB writes in db/adapter.py
3. Adapter inconsistency in framework_adapters/
Generated with Claude Code
Summary by CodeRabbit