fix ACP mcpServers payload (#1735)
* fix ACP mcpServers payload * Handle invalid ACP MCP config
This commit is contained in:
parent
76fad8b08d
commit
48565664e0
|
|
@ -57,6 +57,42 @@ def _build_mcp_servers() -> dict[str, dict[str, Any]]:
|
||||||
return build_servers_config(ExtensionsConfig.from_file())
|
return build_servers_config(ExtensionsConfig.from_file())
|
||||||
|
|
||||||
|
|
||||||
|
def _build_acp_mcp_servers() -> list[dict[str, Any]]:
|
||||||
|
"""Build ACP ``mcpServers`` payload for ``new_session``.
|
||||||
|
|
||||||
|
The ACP client expects a list of server objects, while DeerFlow's MCP helper
|
||||||
|
returns a name -> config mapping for the LangChain MCP adapter. This helper
|
||||||
|
converts the enabled servers into the ACP wire format.
|
||||||
|
"""
|
||||||
|
from deerflow.config.extensions_config import ExtensionsConfig
|
||||||
|
|
||||||
|
extensions_config = ExtensionsConfig.from_file()
|
||||||
|
enabled_servers = extensions_config.get_enabled_mcp_servers()
|
||||||
|
|
||||||
|
mcp_servers: list[dict[str, Any]] = []
|
||||||
|
for name, server_config in enabled_servers.items():
|
||||||
|
transport_type = server_config.type or "stdio"
|
||||||
|
payload: dict[str, Any] = {"name": name, "type": transport_type}
|
||||||
|
|
||||||
|
if transport_type == "stdio":
|
||||||
|
if not server_config.command:
|
||||||
|
raise ValueError(f"MCP server '{name}' with stdio transport requires 'command' field")
|
||||||
|
payload["command"] = server_config.command
|
||||||
|
payload["args"] = server_config.args
|
||||||
|
payload["env"] = [{"name": key, "value": value} for key, value in server_config.env.items()]
|
||||||
|
elif transport_type in ("http", "sse"):
|
||||||
|
if not server_config.url:
|
||||||
|
raise ValueError(f"MCP server '{name}' with {transport_type} transport requires 'url' field")
|
||||||
|
payload["url"] = server_config.url
|
||||||
|
payload["headers"] = [{"name": key, "value": value} for key, value in server_config.headers.items()]
|
||||||
|
else:
|
||||||
|
raise ValueError(f"MCP server '{name}' has unsupported transport type: {transport_type}")
|
||||||
|
|
||||||
|
mcp_servers.append(payload)
|
||||||
|
|
||||||
|
return mcp_servers
|
||||||
|
|
||||||
|
|
||||||
def _build_permission_response(options: list[Any], *, auto_approve: bool) -> Any:
|
def _build_permission_response(options: list[Any], *, auto_approve: bool) -> Any:
|
||||||
"""Build an ACP permission response.
|
"""Build an ACP permission response.
|
||||||
|
|
||||||
|
|
@ -173,7 +209,15 @@ def build_invoke_acp_agent_tool(agents: dict) -> BaseTool:
|
||||||
cmd = agent_config.command
|
cmd = agent_config.command
|
||||||
args = agent_config.args or []
|
args = agent_config.args or []
|
||||||
physical_cwd = _get_work_dir(thread_id)
|
physical_cwd = _get_work_dir(thread_id)
|
||||||
mcp_servers = _build_mcp_servers()
|
try:
|
||||||
|
mcp_servers = _build_acp_mcp_servers()
|
||||||
|
except ValueError as exc:
|
||||||
|
logger.warning(
|
||||||
|
"Invalid MCP server configuration for ACP agent '%s'; continuing without MCP servers: %s",
|
||||||
|
agent,
|
||||||
|
exc,
|
||||||
|
)
|
||||||
|
mcp_servers = []
|
||||||
agent_env: dict[str, str] | None = None
|
agent_env: dict[str, str] | None = None
|
||||||
if agent_config.env:
|
if agent_config.env:
|
||||||
agent_env = {k: (os.environ.get(v[1:], "") if v.startswith("$") else v) for k, v in agent_config.env.items()}
|
agent_env = {k: (os.environ.get(v[1:], "") if v.startswith("$") else v) for k, v in agent_config.env.items()}
|
||||||
|
|
|
||||||
|
|
@ -8,6 +8,7 @@ import pytest
|
||||||
from deerflow.config.acp_config import ACPAgentConfig
|
from deerflow.config.acp_config import ACPAgentConfig
|
||||||
from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig, set_extensions_config
|
from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig, set_extensions_config
|
||||||
from deerflow.tools.builtins.invoke_acp_agent_tool import (
|
from deerflow.tools.builtins.invoke_acp_agent_tool import (
|
||||||
|
_build_acp_mcp_servers,
|
||||||
_build_mcp_servers,
|
_build_mcp_servers,
|
||||||
_build_permission_response,
|
_build_permission_response,
|
||||||
_get_work_dir,
|
_get_work_dir,
|
||||||
|
|
@ -42,6 +43,43 @@ def test_build_mcp_servers_filters_disabled_and_maps_transports():
|
||||||
set_extensions_config(ExtensionsConfig(mcp_servers={}, skills={}))
|
set_extensions_config(ExtensionsConfig(mcp_servers={}, skills={}))
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_acp_mcp_servers_formats_list_payload():
|
||||||
|
set_extensions_config(ExtensionsConfig(mcp_servers={"stale": McpServerConfig(enabled=True, type="stdio", command="echo")}, skills={}))
|
||||||
|
fresh_config = ExtensionsConfig(
|
||||||
|
mcp_servers={
|
||||||
|
"stdio": McpServerConfig(enabled=True, type="stdio", command="npx", args=["srv"], env={"FOO": "bar"}),
|
||||||
|
"http": McpServerConfig(enabled=True, type="http", url="https://example.com/mcp", headers={"Authorization": "Bearer token"}),
|
||||||
|
"disabled": McpServerConfig(enabled=False, type="stdio", command="echo"),
|
||||||
|
},
|
||||||
|
skills={},
|
||||||
|
)
|
||||||
|
monkeypatch = pytest.MonkeyPatch()
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"deerflow.config.extensions_config.ExtensionsConfig.from_file",
|
||||||
|
classmethod(lambda cls: fresh_config),
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
assert _build_acp_mcp_servers() == [
|
||||||
|
{
|
||||||
|
"name": "stdio",
|
||||||
|
"type": "stdio",
|
||||||
|
"command": "npx",
|
||||||
|
"args": ["srv"],
|
||||||
|
"env": [{"name": "FOO", "value": "bar"}],
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"name": "http",
|
||||||
|
"type": "http",
|
||||||
|
"url": "https://example.com/mcp",
|
||||||
|
"headers": [{"name": "Authorization", "value": "Bearer token"}],
|
||||||
|
},
|
||||||
|
]
|
||||||
|
finally:
|
||||||
|
monkeypatch.undo()
|
||||||
|
set_extensions_config(ExtensionsConfig(mcp_servers={}, skills={}))
|
||||||
|
|
||||||
|
|
||||||
def test_build_permission_response_prefers_allow_once():
|
def test_build_permission_response_prefers_allow_once():
|
||||||
response = _build_permission_response(
|
response = _build_permission_response(
|
||||||
[
|
[
|
||||||
|
|
@ -251,9 +289,15 @@ async def test_invoke_acp_agent_uses_fixed_acp_workspace(monkeypatch, tmp_path):
|
||||||
assert captured["spawn"] == {"cmd": "codex-acp", "args": ["--json"], "cwd": expected_cwd}
|
assert captured["spawn"] == {"cmd": "codex-acp", "args": ["--json"], "cwd": expected_cwd}
|
||||||
assert captured["new_session"] == {
|
assert captured["new_session"] == {
|
||||||
"cwd": expected_cwd,
|
"cwd": expected_cwd,
|
||||||
"mcp_servers": {
|
"mcp_servers": [
|
||||||
"github": {"transport": "stdio", "command": "npx", "args": ["github-mcp"]},
|
{
|
||||||
},
|
"name": "github",
|
||||||
|
"type": "stdio",
|
||||||
|
"command": "npx",
|
||||||
|
"args": ["github-mcp"],
|
||||||
|
"env": [],
|
||||||
|
}
|
||||||
|
],
|
||||||
"model": "gpt-5-codex",
|
"model": "gpt-5-codex",
|
||||||
}
|
}
|
||||||
assert captured["prompt"] == {
|
assert captured["prompt"] == {
|
||||||
|
|
@ -448,6 +492,94 @@ async def test_invoke_acp_agent_passes_env_to_spawn(monkeypatch, tmp_path):
|
||||||
assert captured["env"] == {"OPENAI_API_KEY": "sk-from-env", "FOO": "bar"}
|
assert captured["env"] == {"OPENAI_API_KEY": "sk-from-env", "FOO": "bar"}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_invoke_acp_agent_skips_invalid_mcp_servers(monkeypatch, tmp_path, caplog):
|
||||||
|
"""Invalid MCP config should be logged and skipped instead of failing ACP invocation."""
|
||||||
|
from deerflow.config import paths as paths_module
|
||||||
|
|
||||||
|
monkeypatch.setattr(paths_module, "get_paths", lambda: paths_module.Paths(base_dir=tmp_path))
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"deerflow.tools.builtins.invoke_acp_agent_tool._build_acp_mcp_servers",
|
||||||
|
lambda: (_ for _ in ()).throw(ValueError("missing command")),
|
||||||
|
)
|
||||||
|
|
||||||
|
captured: dict[str, object] = {}
|
||||||
|
|
||||||
|
class DummyClient:
|
||||||
|
def __init__(self) -> None:
|
||||||
|
self._chunks: list[str] = []
|
||||||
|
|
||||||
|
@property
|
||||||
|
def collected_text(self) -> str:
|
||||||
|
return ""
|
||||||
|
|
||||||
|
async def session_update(self, session_id, update, **kwargs):
|
||||||
|
pass
|
||||||
|
|
||||||
|
async def request_permission(self, options, session_id, tool_call, **kwargs):
|
||||||
|
raise AssertionError("should not be called")
|
||||||
|
|
||||||
|
class DummyConn:
|
||||||
|
async def initialize(self, **kwargs):
|
||||||
|
pass
|
||||||
|
|
||||||
|
async def new_session(self, **kwargs):
|
||||||
|
captured["new_session"] = kwargs
|
||||||
|
return SimpleNamespace(session_id="s1")
|
||||||
|
|
||||||
|
async def prompt(self, **kwargs):
|
||||||
|
pass
|
||||||
|
|
||||||
|
class DummyProcessContext:
|
||||||
|
def __init__(self, client, cmd, *args, env=None, cwd=None):
|
||||||
|
captured["spawn"] = {"cmd": cmd, "args": list(args), "env": env, "cwd": cwd}
|
||||||
|
|
||||||
|
async def __aenter__(self):
|
||||||
|
return DummyConn(), object()
|
||||||
|
|
||||||
|
async def __aexit__(self, exc_type, exc, tb):
|
||||||
|
return False
|
||||||
|
|
||||||
|
class DummyRequestError(Exception):
|
||||||
|
@staticmethod
|
||||||
|
def method_not_found(method):
|
||||||
|
return DummyRequestError(method)
|
||||||
|
|
||||||
|
monkeypatch.setitem(
|
||||||
|
sys.modules,
|
||||||
|
"acp",
|
||||||
|
SimpleNamespace(
|
||||||
|
PROTOCOL_VERSION="2026-03-24",
|
||||||
|
Client=DummyClient,
|
||||||
|
RequestError=DummyRequestError,
|
||||||
|
spawn_agent_process=lambda client, cmd, *args, env=None, cwd: DummyProcessContext(client, cmd, *args, env=env, cwd=cwd),
|
||||||
|
text_block=lambda text: {"type": "text", "text": text},
|
||||||
|
),
|
||||||
|
)
|
||||||
|
monkeypatch.setitem(
|
||||||
|
sys.modules,
|
||||||
|
"acp.schema",
|
||||||
|
SimpleNamespace(
|
||||||
|
ClientCapabilities=lambda: {},
|
||||||
|
Implementation=lambda **kwargs: kwargs,
|
||||||
|
TextContentBlock=type("TextContentBlock", (), {"__init__": lambda self, text: setattr(self, "text", text)}),
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
tool = build_invoke_acp_agent_tool({"codex": ACPAgentConfig(command="codex-acp", description="Codex CLI")})
|
||||||
|
caplog.set_level("WARNING")
|
||||||
|
|
||||||
|
try:
|
||||||
|
await tool.coroutine(agent="codex", prompt="Do something")
|
||||||
|
finally:
|
||||||
|
sys.modules.pop("acp", None)
|
||||||
|
sys.modules.pop("acp.schema", None)
|
||||||
|
|
||||||
|
assert captured["new_session"]["mcp_servers"] == []
|
||||||
|
assert "continuing without MCP servers" in caplog.text
|
||||||
|
assert "missing command" in caplog.text
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.anyio
|
@pytest.mark.anyio
|
||||||
async def test_invoke_acp_agent_passes_none_env_when_not_configured(monkeypatch, tmp_path):
|
async def test_invoke_acp_agent_passes_none_env_when_not_configured(monkeypatch, tmp_path):
|
||||||
"""When env is empty, None is passed to spawn_agent_process (subprocess inherits parent env)."""
|
"""When env is empty, None is passed to spawn_agent_process (subprocess inherits parent env)."""
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue