-
-
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 3 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,31 @@ | ||
| """Real agentic test: --external-agent uses manager Agent delegation by default.""" | ||
| import os | ||
| import shutil | ||
| import subprocess | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not shutil.which("claude"), reason="claude CLI not installed") | ||
| @pytest.mark.skipif(not os.getenv("OPENAI_API_KEY"), reason="OPENAI_API_KEY required for manager LLM") | ||
| def test_external_agent_manager_delegation_default(): | ||
| """Real agentic test: default --external-agent uses manager Agent + subagent tool.""" | ||
| result = subprocess.run( | ||
| ["praisonai", "Say hi in exactly 5 words", "--external-agent", "claude"], | ||
| capture_output=True, text=True, timeout=120, | ||
| ) | ||
| assert result.returncode == 0 | ||
| assert "manager delegation" in result.stdout.lower() | ||
| assert len(result.stdout.strip()) > 0 | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not shutil.which("claude"), reason="claude CLI not installed") | ||
| def test_external_agent_direct_flag_preserves_proxy(): | ||
| """Escape hatch: --external-agent-direct preserves pass-through proxy.""" | ||
| result = subprocess.run( | ||
| ["praisonai", "Say hi in exactly 5 words", "--external-agent", "claude", | ||
| "--external-agent-direct"], | ||
| capture_output=True, text=True, timeout=120, | ||
| ) | ||
| assert result.returncode == 0 | ||
| assert "direct" in result.stdout.lower() | ||
| assert len(result.stdout.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: