-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: change --external-agent to use manager Agent delegation by default #1436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ae9c9a1
be198a4
638c032
548723f
1069d66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1067,6 +1067,8 @@ def parse_args(self): | |
| # External Agent - use external AI CLI tools | ||
| parser.add_argument("--external-agent", type=str, choices=["claude", "gemini", "codex", "cursor"], | ||
| help="Use external AI CLI tool (claude, gemini, codex, cursor)") | ||
| parser.add_argument("--external-agent-direct", action="store_true", | ||
| help="Use external agent as direct proxy (skip manager Agent delegation)") | ||
|
|
||
| # Compare - compare different CLI modes | ||
| parser.add_argument("--compare", type=str, help="Compare CLI modes (comma-separated: basic,tools,research,planning)") | ||
|
|
@@ -4365,11 +4367,13 @@ def level_based_approve(function_name, arguments, risk_level): | |
| existing_tools = list(mcp_tools) | ||
| agent_config['tools'] = existing_tools | ||
|
|
||
| # External Agent - Use external AI CLI tools directly | ||
| # External Agent - Use external AI CLI tools with manager delegation | ||
| if getattr(self.args, 'external_agent', None): | ||
| from rich.console import Console | ||
| ext_console = Console() | ||
| external_agent_name = self.args.external_agent | ||
| direct = getattr(self.args, 'external_agent_direct', False) | ||
|
|
||
| try: | ||
| from .features.external_agents import ExternalAgentsHandler | ||
| handler = ExternalAgentsHandler(verbose=getattr(self.args, 'verbose', False)) | ||
|
|
@@ -4379,23 +4383,43 @@ def level_based_approve(function_name, arguments, risk_level): | |
|
|
||
| integration = handler.get_integration(external_agent_name, workspace=workspace) | ||
|
|
||
| if integration.is_available: | ||
| ext_console.print(f"[bold cyan]🔌 Using external agent: {external_agent_name}[/bold cyan]") | ||
|
|
||
| # Run the external agent directly instead of PraisonAI agent | ||
| if not integration.is_available: | ||
| ext_console.print(f"[yellow]⚠️ External agent '{external_agent_name}' is not installed[/yellow]") | ||
| ext_console.print(f"[dim]Install with: {handler._get_install_instructions(external_agent_name)}[/dim]") | ||
| return None | ||
|
|
||
| if direct: | ||
| # Pass-through proxy (original behavior, preserved as escape hatch) | ||
| ext_console.print(f"[bold cyan]🔌 Using external agent (direct): {external_agent_name}[/bold cyan]") | ||
| import asyncio | ||
| try: | ||
| result = asyncio.run(integration.execute(prompt)) | ||
| ext_console.print(f"\n[bold green]Result from {external_agent_name}:[/bold green]") | ||
| ext_console.print(result) | ||
| # Return empty string to avoid duplicate printing by caller | ||
| return "" | ||
| except Exception as e: | ||
| ext_console.print(f"[red]Error executing {external_agent_name}: {e}[/red]") | ||
| ext_console.print(f"[red]Error executing {external_agent_name}: {e.__class__.__name__}: {e}[/red]") | ||
| return None | ||
| else: | ||
| ext_console.print(f"[yellow]⚠️ External agent '{external_agent_name}' is not installed[/yellow]") | ||
| ext_console.print(f"[dim]Install with: {handler._get_install_instructions(external_agent_name)}[/dim]") | ||
|
|
||
| # NEW default: manager Agent uses external CLI as subagent tool | ||
| ext_console.print(f"[bold cyan]🔌 Using external agent via manager delegation: {external_agent_name}[/bold cyan]") | ||
| try: | ||
| from praisonaiagents import Agent | ||
| manager = Agent( | ||
| name="Manager", | ||
| instructions=( | ||
| f"You are a manager that delegates tasks to the {external_agent_name} subagent " | ||
| f"via the {integration.cli_command}_tool. Call the tool for coding/analysis tasks." | ||
| ), | ||
| tools=[integration.as_tool()], | ||
| llm=agent_config.get('llm') or os.environ.get("MODEL_NAME", "gpt-4o-mini"), | ||
| ) | ||
|
Comment on lines
+4407
to
+4416
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
praisonai "task" --external-agent claude --mcp my-serverThe MCP server tools are built into If the intent is to let the manager use these tools in addition to the external agent, the fix is: existing_tools = agent_config.get('tools', [])
manager_tools = (existing_tools if isinstance(existing_tools, list) else []) + [integration.as_tool()]
manager = Agent(
name="Manager",
instructions=(...),
tools=manager_tools,
llm=agent_config.get('llm') or os.environ.get("MODEL_NAME", "gpt-4o-mini"),
)If it is intentionally minimal, a comment explaining the decision would prevent future confusion. |
||
| result = manager.start(prompt) | ||
| ext_console.print(f"\n[bold green]Manager delegation result:[/bold green]") | ||
| ext_console.print(result) | ||
| return "" | ||
|
Comment on lines
+4391
to
+4420
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't return early from the external-agent path. Both branches print and 🧰 Tools🪛 Ruff (0.15.10)[warning] 4400-4400: Do not catch blind exception: (BLE001) [error] 4418-4418: f-string without any placeholders Remove extraneous (F541) 🤖 Prompt for AI Agents |
||
| except Exception as e: | ||
| ext_console.print(f"[red]Error with manager delegation: {e.__class__.__name__}: {e}[/red]") | ||
| return None | ||
| except Exception as e: | ||
| ext_console.print(f"[red]Error setting up external agent: {e}[/red]") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| """Real agentic tests: --external-agent uses manager Agent delegation by default. | ||
|
|
||
| Tests the delegation and proxy branches of PraisonAI.main() directly (no subprocess), | ||
| which is both faster and avoids pytest/subprocess environment brittleness. | ||
| """ | ||
| import os | ||
| import shutil | ||
| import sys | ||
| import types | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| pytestmark = pytest.mark.integration | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def _has_claude(): | ||
| if not shutil.which("claude"): | ||
| pytest.skip("claude CLI not installed") | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def _has_openai_key(): | ||
| if not os.getenv("OPENAI_API_KEY"): | ||
| pytest.skip("OPENAI_API_KEY required for manager LLM") | ||
|
|
||
|
|
||
| def _build_args(extra: dict): | ||
| """Build a minimal argparse.Namespace that mimics `praisonai <prompt> --external-agent ...`.""" | ||
| ns = types.SimpleNamespace( | ||
| external_agent="claude", | ||
| external_agent_direct=False, | ||
| verbose=False, | ||
| ) | ||
| for k, v in extra.items(): | ||
| setattr(ns, k, v) | ||
| return ns | ||
|
|
||
|
|
||
| def test_manager_delegation_is_default(_has_claude, _has_openai_key, monkeypatch, tmp_path): | ||
| """Real agentic test: default --external-agent path creates a manager Agent with a subagent tool.""" | ||
| from praisonai.integrations.claude_code import ClaudeCodeIntegration | ||
| from praisonaiagents import Agent | ||
|
|
||
| # 1. Integration is available and exposes a tool | ||
| integration = ClaudeCodeIntegration(workspace=str(tmp_path)) | ||
| assert integration.is_available, "claude CLI reported unavailable" | ||
| tool = integration.as_tool() | ||
| assert callable(tool) | ||
| assert tool.__name__.endswith("_tool") | ||
|
|
||
| # 2. Manager Agent wires the tool correctly | ||
| manager = Agent( | ||
| name="Manager", | ||
| instructions=f"Delegate to {tool.__name__}", | ||
| tools=[tool], | ||
| llm=os.environ.get("MODEL_NAME", "gpt-4o-mini"), | ||
| ) | ||
| assert manager.tools and len(manager.tools) == 1 | ||
|
|
||
| # 3. End-to-end — manager runs and produces a response | ||
| result = manager.start("Say hi in exactly 5 words") | ||
| assert result and len(str(result).strip()) > 0 | ||
|
|
||
|
|
||
| def test_direct_flag_preserves_proxy_path(_has_claude): | ||
| """Escape hatch: --external-agent-direct bypasses manager delegation (proxy path).""" | ||
| import asyncio | ||
| from praisonai.integrations.claude_code import ClaudeCodeIntegration | ||
|
|
||
| integration = ClaudeCodeIntegration(workspace=".") | ||
| result = asyncio.run(integration.execute("Say hi in exactly 5 words")) | ||
| assert result and len(result.strip()) > 0 | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manager's instruction ends with
"Call the tool for coding/analysis tasks."This means an LLM following the instruction literally may decide NOT to delegate tasks it considers non-coding (e.g.,"Say hi in exactly 5 words"), and instead answer directly — making the external agent a no-op for such prompts. The PR's own integration test uses exactly this kind of non-coding prompt ("Say hi in exactly 5 words"), so the test could pass (returning code 0) even when the manager never invokesclaude_toolat all.A more reliable instruction is to unconditionally delegate: