fix: correct callback signature mismatch in multi-agent startup (fixes #1730)#1740
fix: correct callback signature mismatch in multi-agent startup (fixes #1730)#1740MervinPraison wants to merge 3 commits into
Conversation
…#1730) - Fix composed_on_task_start to accept both task and task_id parameters - Fix composed_on_task_complete to accept both task and task_output parameters - Ensures callbacks receive the correct number of arguments as expected - Resolves AgentTeam startup error with function signature mismatch Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
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? |
📝 WalkthroughWalkthroughFixed verbose-mode callback wrappers in AgentTeam.start() to forward full parameters: ChangesVerbose Callback Parameter Alignment
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 fixes a signature mismatch in
Confidence Score: 5/5The change is a minimal, targeted correction to two function signatures in a single verbose-mode code path; no data flow or task execution logic is altered. Both composed callback wrappers now match the actual call sites in run_task (lines 1310 and 1401), which have always passed two arguments. The verbose-display helper functions already expected two arguments, so the fix is internally consistent. The accompanying unit test directly exercises the repaired path and would have caught the original bug. No other logic is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as start(output="verbose")
participant CS as composed_on_task_start
participant VS as verbose_task_start_callback
participant UCS as original_on_task_start (user)
participant CC as composed_on_task_complete
participant VC as verbose_task_complete_callback
participant UCC as original_on_task_complete (user)
Caller->>CS: (task, task_id) ← fixed
CS->>VS: (task, task_id) ← fixed
VS-->>CS: renders Rich panel
CS->>UCS: (task, task_id) ← fixed
Note over Caller,UCC: Task executes …
Caller->>CC: (task, task_output) ← fixed
CC->>VC: (task, task_output) ← fixed
VC-->>CC: renders response panel
CC->>UCC: (task, task_output) ← fixed
Reviews (2): Last reviewed commit: "fix: forward verbose callback args and a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a multi-agent startup failure caused by a callback signature mismatch in the verbose-mode callback composition inside Agents.start(), aligning composed callbacks with the runtime invocation (on_task_start(task, task_id) / on_task_complete(task, task_output)).
Changes:
- Update
composed_on_task_startto accept(task, task_id)and forward both arguments to the original callback. - Update
composed_on_task_completeto accept(task, task_output)and forward both arguments to the original callback.
Comments suppressed due to low confidence (1)
src/praisonai-agents/praisonaiagents/agents/agents.py:1635
- This callback-signature regression appears to have been user-facing (fixes #1730), but there are currently no tests covering the multi-agent hook signatures (e.g.,
on_task_start(task, task_id)and the verbose-mode callback composition). Adding a unit test that runsAgents.start()(or the underlying task runner) with a customhooks.on_task_start/on_task_completeandshow_verbose=Truewould prevent reintroducing this mismatch.
# Set callbacks for verbose display (compose with existing callbacks)
original_on_task_start = self.on_task_start
original_on_task_complete = self.on_task_complete
def composed_on_task_start(task, task_id):
try:
verbose_task_start_callback(task)
except Exception as e:
logging.debug(f"Error in verbose task start callback: {e}")
if original_on_task_start:
try:
original_on_task_start(task, task_id)
except Exception as e:
logging.debug(f"Error in original task start callback: {e}")
def composed_on_task_complete(task, task_output):
try:
verbose_task_complete_callback(task)
except Exception as e:
logging.debug(f"Error in verbose task complete callback: {e}")
if original_on_task_complete:
try:
original_on_task_complete(task, task_output)
except Exception as e:
logging.debug(f"Error in original task complete callback: {e}")
self.on_task_start = composed_on_task_start
self.on_task_complete = composed_on_task_complete
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def composed_on_task_start(task, task_id): | ||
| try: | ||
| verbose_task_start_callback(task) | ||
| except Exception as e: |
| def composed_on_task_complete(task): | ||
| def composed_on_task_complete(task, task_output): | ||
| try: | ||
| verbose_task_complete_callback(task) |
|
@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 Final Architecture Review Complete ✅Phase 1: Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
🎯 FINAL ARCHITECTURAL VERDICT: APPROVED ✅✅ Original Issue Resolution ✅ Additional Critical Fix Applied
✅ Architecture Compliance
📁 Files Modified:
🚫 Issues Skipped:
✅ Multi-Agent Startup: Now works without crashes This PR resolves the critical startup issue while maintaining architectural integrity and improving verbose mode functionality. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/agents/agents.py`:
- Around line 1612-1615: The composed_on_task_start and
composed_on_task_complete wrappers are calling the verbose handlers with only
one argument, but the verbose callbacks expect two; update
composed_on_task_start to call verbose_task_start_callback(task, task_id) and
update composed_on_task_complete to call verbose_task_complete_callback(task,
task_id) (or pass the exact second arg used by those wrappers) so the full
callback arguments are forwarded to the verbose wrappers (keep the existing
try/except behavior).
🪄 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: 73228a5f-a1ec-452b-a040-1f83531a872c
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/agents/agents.py
…isplay issue) - Pass task_id to verbose_task_start_callback(task, task_id) - Pass task_output to verbose_task_complete_callback(task, task_output) - Resolves silent TypeError in verbose mode identified by all reviewers Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
![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 updates the task callback wrappers in agents.py to include task_id and task_output parameters, ensuring they are passed to the original callbacks. However, the reviewer identified that the internal calls to verbose_task_start_callback and verbose_task_complete_callback were not updated with these new arguments, which will result in TypeError exceptions when verbose mode is enabled.
| original_on_task_complete = self.on_task_complete | ||
|
|
||
| def composed_on_task_start(task): | ||
| def composed_on_task_start(task, task_id): |
There was a problem hiding this comment.
While the signature of composed_on_task_start is correctly updated to accept task_id, the call to verbose_task_start_callback(task) on line 1614 (within the same function) is still missing the task_id argument. Since verbose_task_start_callback is defined to expect two arguments (line 1561), this will result in a TypeError when verbose mode is enabled.
| logging.debug(f"Error in original task start callback: {e}") | ||
|
|
||
| def composed_on_task_complete(task): | ||
| def composed_on_task_complete(task, task_output): |
There was a problem hiding this comment.
While the signature of composed_on_task_complete is correctly updated to accept task_output, the call to verbose_task_complete_callback(task) on line 1625 (within the same function) is still missing the task_output argument. Since verbose_task_complete_callback is defined to expect two arguments (line 1586), this will result in a TypeError when verbose mode is enabled.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/bd0c3ea8-33b8-4148-b703-2c368de0aa14 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Addressed and validated in commit Findings after reviewing all available bot feedback above:
Review summary for this PR:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/tests/unit/agents/test_start_verbose_callbacks.py`:
- Around line 29-45: The test must be replaced or supplemented by an end-to-end
integration test that calls AgentTeam.start() through the real agentic path (no
monkeypatch of run_all_tasks or stdout), supplies a real prompt to an Agent via
Agent.start()/AgentTeam.start(), and lets the configured LLM client make an
actual API/model call so the agent produces real text output; implement a new
integration test that constructs a minimal AgentTeam with a real LLM provider
(use the same LLM client class the repo uses for integration tests, e.g., the
OpenAI/Anthropic client wrapper used elsewhere), ensure required API key env
vars are set in the test, call team.start(output="verbose") with a concrete
prompt, wait for completion, and assert the final callback/on_complete receives
a non-empty text response and that debug logs do not contain "Error in verbose
task"; do not monkeypatch run_all_tasks or logging.debug in this test so the
real agent path is exercised.
🪄 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: 5be0f1ae-df3c-477e-8487-a6fad7858344
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/agents/agents.pysrc/praisonai-agents/tests/unit/agents/test_start_verbose_callbacks.py
| def fake_run_all_tasks(): | ||
| task_id, task = next(iter(team.tasks.items())) | ||
| task_output = SimpleNamespace(raw="done") | ||
| team.on_task_start(task, task_id) | ||
| team.on_task_complete(task, task_output) | ||
|
|
||
| monkeypatch.setattr(team, "run_all_tasks", fake_run_all_tasks) | ||
| monkeypatch.setattr(sys.stdout, "isatty", lambda: True) | ||
| monkeypatch.setattr("praisonaiagents.agents.agents.logging.debug", lambda msg: debug_messages.append(str(msg))) | ||
|
|
||
| team.start(output="verbose") | ||
|
|
||
| assert len(on_start_calls) == 1 | ||
| assert len(on_complete_calls) == 1 | ||
| assert on_start_calls[0][1] == next(iter(team.tasks.keys())) | ||
| assert on_complete_calls[0][1].raw == "done" | ||
| assert all("Error in verbose task" not in message for message in debug_messages) |
There was a problem hiding this comment.
Add mandatory real agentic coverage for this feature.
This test is valuable for callback wiring, but it does not satisfy the required real agentic path (real prompt + LLM call + actual text response). Please add an integration/e2e test that exercises AgentTeam.start() end-to-end for this callback fix.
As per coding guidelines, "Real agentic tests are MANDATORY for every feature: Agent must call agent.start() with a real prompt, call the LLM, and produce actual text response—not just smoke tests of object construction".
🤖 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-agents/tests/unit/agents/test_start_verbose_callbacks.py`
around lines 29 - 45, The test must be replaced or supplemented by an end-to-end
integration test that calls AgentTeam.start() through the real agentic path (no
monkeypatch of run_all_tasks or stdout), supplies a real prompt to an Agent via
Agent.start()/AgentTeam.start(), and lets the configured LLM client make an
actual API/model call so the agent produces real text output; implement a new
integration test that constructs a minimal AgentTeam with a real LLM provider
(use the same LLM client class the repo uses for integration tests, e.g., the
OpenAI/Anthropic client wrapper used elsewhere), ensure required API key env
vars are set in the test, call team.start(output="verbose") with a concrete
prompt, wait for completion, and assert the final callback/on_complete receives
a non-empty text response and that debug logs do not contain "Error in verbose
task"; do not monkeypatch run_all_tasks or logging.debug in this test so the
real agent path is exercised.
Fixes #1730
Summary
Root Cause
The error occurred because:
self.on_task_start(task, task_id)composed_on_task_startonly accepted 1 argumentTesting
✅ Verified the original user code now works without the signature error
✅ Multi-agent setup initializes successfully
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests