From 2e0b591076eb4f0d3839123d95bbd71893fc7163 Mon Sep 17 00:00:00 2001 From: nikshepsvn Date: Sun, 17 May 2026 17:44:42 +0530 Subject: [PATCH] fix(tools): validate acp_command binary exists before forcing copilot-acp transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/tools/test_delegate.py | 85 ++++++++++++++++++++++++++++++++++++ tools/delegate_tool.py | 16 +++++++ 2 files changed, 101 insertions(+) diff --git a/tests/tools/test_delegate.py b/tests/tools/test_delegate.py index 5eef8f3bb..6231e0303 100644 --- a/tests/tools/test_delegate.py +++ b/tests/tools/test_delegate.py @@ -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.""" diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 7bd510d7f..72559b1a5 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -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 )