fix(tools): validate acp_command binary exists before forcing copilot-acp transport
When a model passes `acp_command="copilot"` (or any other binary name) in a `delegate_task` tool call, `_build_child_agent` unconditionally sets `effective_provider = "copilot-acp"`, which routes the subagent through `CopilotACPClient`. That client spawns the named binary via subprocess; if it isn't on PATH, every retry raises RuntimeError and an asyncio cleanup race during error delivery can take the entire gateway down. This is a real failure mode on headless deploys (Railway / Fly / VPS / Docker) where `copilot` / `claude` / etc. aren't installed. The schema does say "Do NOT set unless the user explicitly told you an ACP CLI is installed," but models occasionally pass it anyway — particularly for X (Twitter) search prompts where Grok seems to associate ACP with "search assistance." Reproduction: - Headless install (no `copilot` binary on PATH) - Set provider to xai-oauth + model grok-4.3 - Telegram prompt: "Search X for crypto twitter trends" - Grok decides to delegate and passes `acp_command="copilot"` - Subagent crashes 3x, gateway crashes on the 3rd retry teardown Fix: validate the binary exists on PATH via `shutil.which` before honoring the override. If missing, log a warning and fall through to the parent's default transport. No behavior change when the binary IS present (covered by `test_build_child_agent_honors_acp_command_when_binary_present`). Tests: - `test_build_child_agent_ignores_acp_command_when_binary_missing` - `test_build_child_agent_honors_acp_command_when_binary_present` Verified on Python 3.11 (macOS) and 3.12 (Debian 13 container). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6d6702ef50
commit
2e0b591076
2 changed files with 101 additions and 0 deletions
|
|
@ -517,6 +517,91 @@ class TestToolNamePreservation(unittest.TestCase):
|
|||
f"_saved_tool_names leaked back into wrong scope: {exc}"
|
||||
)
|
||||
|
||||
def test_build_child_agent_ignores_acp_command_when_binary_missing(self):
|
||||
"""Regression: _build_child_agent must not force provider='copilot-acp'
|
||||
when the override_acp_command binary is not on PATH.
|
||||
|
||||
Without this guard, a model that hallucinates
|
||||
``delegate_task(acp_command="copilot")`` on a host without the Copilot
|
||||
CLI installed (Railway / headless containers / fresh VPS) would route
|
||||
the subagent through CopilotACPClient, which spawns the binary via
|
||||
subprocess and raises RuntimeError. After 3 retries the asyncio loop
|
||||
teardown can take the entire gateway down.
|
||||
"""
|
||||
parent = _make_mock_parent(depth=0)
|
||||
# The crash scenario is a TG/cron agent on a host with no ACP CLI —
|
||||
# parent itself has no acp_command, so clearing the override must NOT
|
||||
# fall through to a stray parent value.
|
||||
parent.acp_command = None
|
||||
parent.acp_args = []
|
||||
captured = {}
|
||||
|
||||
with patch("run_agent.AIAgent") as MockAgent, \
|
||||
patch("shutil.which", return_value=None) as mock_which:
|
||||
mock_child = MagicMock()
|
||||
MockAgent.return_value = mock_child
|
||||
|
||||
_build_child_agent(
|
||||
task_index=0,
|
||||
goal="search X for crypto twitter",
|
||||
context=None,
|
||||
toolsets=None,
|
||||
model=None,
|
||||
max_iterations=10,
|
||||
parent_agent=parent,
|
||||
task_count=1,
|
||||
override_acp_command="copilot",
|
||||
override_acp_args=["--foo"],
|
||||
)
|
||||
|
||||
_, kwargs = MockAgent.call_args
|
||||
captured["provider"] = kwargs.get("provider")
|
||||
captured["acp_command"] = kwargs.get("acp_command")
|
||||
captured["acp_args"] = kwargs.get("acp_args")
|
||||
|
||||
mock_which.assert_called_with("copilot")
|
||||
self.assertNotEqual(
|
||||
captured["provider"],
|
||||
"copilot-acp",
|
||||
"missing acp_command binary must NOT force copilot-acp provider",
|
||||
)
|
||||
self.assertIsNone(captured["acp_command"])
|
||||
self.assertEqual(captured["acp_args"], [])
|
||||
|
||||
def test_build_child_agent_honors_acp_command_when_binary_present(self):
|
||||
"""When the acp_command binary exists on PATH, behavior is unchanged:
|
||||
provider is forced to copilot-acp and command/args propagate to the
|
||||
child agent. Guards against the missing-binary check accidentally
|
||||
breaking working ACP delegation setups.
|
||||
"""
|
||||
parent = _make_mock_parent(depth=0)
|
||||
captured = {}
|
||||
|
||||
with patch("run_agent.AIAgent") as MockAgent, \
|
||||
patch("shutil.which", return_value="/usr/local/bin/copilot"):
|
||||
mock_child = MagicMock()
|
||||
MockAgent.return_value = mock_child
|
||||
|
||||
_build_child_agent(
|
||||
task_index=0,
|
||||
goal="copilot path",
|
||||
context=None,
|
||||
toolsets=None,
|
||||
model=None,
|
||||
max_iterations=10,
|
||||
parent_agent=parent,
|
||||
task_count=1,
|
||||
override_acp_command="copilot",
|
||||
override_acp_args=["--foo"],
|
||||
)
|
||||
|
||||
_, kwargs = MockAgent.call_args
|
||||
captured["provider"] = kwargs.get("provider")
|
||||
captured["acp_command"] = kwargs.get("acp_command")
|
||||
|
||||
self.assertEqual(captured["provider"], "copilot-acp")
|
||||
self.assertEqual(captured["acp_command"], "copilot")
|
||||
|
||||
def test_saved_tool_names_set_on_child_before_run(self):
|
||||
"""_run_single_child must set _delegate_saved_tool_names on the child
|
||||
from model_tools._last_resolved_tool_names before run_conversation."""
|
||||
|
|
|
|||
|
|
@ -1229,6 +1229,22 @@ def _build_child_agent(
|
|||
effective_api_mode = None # force re-derivation from provider's defaults
|
||||
else:
|
||||
effective_api_mode = getattr(parent_agent, "api_mode", None)
|
||||
# Defensive: validate override_acp_command exists on PATH before honoring
|
||||
# it. Models occasionally pass acp_command="copilot" / "claude" / etc. in
|
||||
# delegate_task tool calls despite the schema saying not to, which forces
|
||||
# the subagent onto the copilot-acp transport below and crashes the
|
||||
# gateway when the binary is missing (e.g. headless container deploys).
|
||||
if override_acp_command:
|
||||
import shutil as _shutil
|
||||
|
||||
if not _shutil.which(override_acp_command):
|
||||
logger.warning(
|
||||
"Ignoring acp_command=%r: binary not found on PATH; "
|
||||
"falling back to default transport.",
|
||||
override_acp_command,
|
||||
)
|
||||
override_acp_command = None
|
||||
override_acp_args = None
|
||||
effective_acp_command = override_acp_command or getattr(
|
||||
parent_agent, "acp_command", None
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue