Files
q792602257andClaude Opus 4.6 70e0624a47 fix(host-agent): align MCP integration with mcp SDK 1.28.1 realities
Three final-review deviations closed:

I1 (session-end release): mcp SDK 1.28.1 exposes no per-session
shutdown callback (only a server-level lifespan). Lower the
McpBusyTracker default TTL from 60s to 20s and update spec §6.5,
Q5/R3, D9, and docs/MCP_INTEGRATION.md concurrency section to
document the TTL-only recovery path. 20s is short enough to recover
within one 30s heartbeat interval but long enough that an active
session does not lose its lease during normal operator pauses.

I2 (JSON-RPC error shape): FastMCP Tool.run wraps every non-
UrlElicitationRequiredError exception (including McpError with typed
ErrorData) into ToolError, which the lowlevel call_tool handler
serializes as CallToolResult(isError=true, content=[TextContent(...)]).
There is no public path that surfaces JSON-RPC -32000 with structured
data.busy_owner from a tool call site. Update spec §7 error matrix
and docs/MCP_INTEGRATION.md error table to document the actual wire
shape; busy_owner now lives in the text content.

I3 (typing): mcp_server: Any = None -> FastMCP | None = None via
TYPE_CHECKING, keeping the mcp import lazy (matches precedent
elsewhere in the codebase) while adding static type checking at the
create_console_app boundary.

Tests added (4):
- test_default_ttl_is_20_seconds — locks I1's new default TTL
- test_default_ttl_recovers_dead_session_within_one_window — locks
  I1's recovery semantics (lease sweeped on next read after 20s)
- test_busy_error_wire_shape_is_calltoolresult_iserror — pins I2's
  wire envelope via Tool.run + lowlevel Server._make_error_result
- test_busy_error_text_includes_cloud_assignment_owner — same for
  the cloud_assignment busy_owner branch

Full non-integration suite: 697 passed / 54 deselected (was 693 / 54).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-07-21 16:25:35 +08:00

416 lines
14 KiB
Python

from __future__ import annotations
from datetime import UTC, datetime
from typing import Any
import pytest
from cloud.internal_api.models import AssignmentModel
from device.manager import DeviceManager
from driver.base import Driver
from host_agent.mcp_lock import McpBusyTracker
from host_agent.status import AgentStatusTracker
from host_agent.web.mcp import (
McpDeviceBusyError,
_call_tool_sync,
_current_session_id,
build_mcp_server,
)
from mcp.server.fastmcp import Context
from mcp.shared.context import RequestContext
class _FakeDriver(Driver):
"""Minimal driver. connect/screenshot/tap are exercised; remaining abstract
methods are stubbed to satisfy Driver's ABC contract."""
def __init__(self) -> None:
self.taps: list[tuple[float, float]] = []
def connect(self) -> None:
return None
def disconnect(self) -> None:
return None
def screenshot(self) -> bytes:
return b"fake"
def tap(self, x: float, y: float) -> None:
self.taps.append((x, y))
def long_press(self, x: float, y: float, duration_ms: int = 1200) -> None:
return None
def swipe(
self,
start_x: float,
start_y: float,
end_x: float,
end_y: float,
duration_ms: int = 500,
) -> None:
return None
def swipe_path(
self, waypoints: list[tuple[float, float]], duration_ms: int
) -> None:
return None
def double_tap(self, x: float, y: float, interval_ms: int = 80) -> None:
return None
def input(self, text: str) -> None:
return None
def launch(self, app_id: str) -> None:
return None
def terminate(self, app_id: str) -> None:
return None
def tree(self) -> Any:
return None
def home(self) -> None:
return None
def lock(self) -> None:
return None
def unlock(self) -> None:
return None
def _make_manager_with_device(device_id: str = "phone-1") -> DeviceManager:
manager = DeviceManager()
manager.register_device(
device_id=device_id,
driver_factory=lambda: _FakeDriver(),
name=device_id,
)
manager.connect(device_id)
return manager
def test_build_mcp_server_returns_fastmcp_instance() -> None:
from mcp.server.fastmcp import FastMCP
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
assert isinstance(server, FastMCP)
def test_call_tool_succeeds_when_device_is_free() -> None:
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
result = _call_tool_sync(
server, "take_screenshot", {"device_id": "phone-1"}, session_id="sess-a"
)
assert result["ok"] is True
assert "phone-1" in tracker.busy_device_ids()
def test_call_tool_fails_when_cloud_uses_device() -> None:
"""AgentStatusTracker.current_assignment.device_id matches -> busy."""
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
status.mark_assignment_started(
AssignmentModel(
task_id="t1",
attempt=1,
lease_id="l1",
lease_expires_at=datetime.now(UTC),
host_id="h1",
device_id="phone-1",
goal="cloud task",
)
)
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
with pytest.raises(McpDeviceBusyError) as exc:
_call_tool_sync(
server,
"take_screenshot",
{"device_id": "phone-1"},
session_id="sess-a",
)
assert exc.value.device_id == "phone-1"
assert exc.value.busy_owner == "cloud_assignment"
def test_call_tool_fails_when_another_mcp_session_holds_device() -> None:
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
# Pre-acquire as a different session.
tracker.acquire("phone-1", "sess-other")
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
with pytest.raises(McpDeviceBusyError) as exc:
_call_tool_sync(
server,
"take_screenshot",
{"device_id": "phone-1"},
session_id="sess-a",
)
assert exc.value.busy_owner.startswith("mcp_session:")
def test_call_tool_renews_when_same_session_already_holds() -> None:
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
_call_tool_sync(
server, "take_screenshot", {"device_id": "phone-1"}, session_id="sess-a"
)
# Second call from the same session should succeed.
result = _call_tool_sync(
server, "take_screenshot", {"device_id": "phone-1"}, session_id="sess-a"
)
assert result["ok"] is True
def test_list_devices_uses_display_status() -> None:
"""Connected-but-idle devices report as 'connected', not 'busy'."""
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
result = _call_tool_sync(server, "list_devices", {}, session_id="sess-a")
assert isinstance(result, list)
assert result[0]["status"] == "connected"
def test_unknown_device_returns_semantic_error_dict() -> None:
"""take_screenshot against an unknown device returns the api-errors semantic
error dict (``ok=False, error="device not found"``) rather than raising.
Note: this test adapts the brief's exception-assertion semantics to the
actual behavior of ``call_with_semantic_errors`` in ``api/errors.py`` —
the brief's expectation that an exception is raised here is incorrect for
the current handler implementation."""
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
result = _call_tool_sync(
server,
"take_screenshot",
{"device_id": "does-not-exist"},
session_id="sess-a",
)
assert isinstance(result, dict)
assert result["ok"] is False
assert "device" in result["error"].lower()
def test_manager_required_for_build_mcp_server() -> None:
"""Reinforces D12 — build_mcp_server requires manager as keyword-only."""
import inspect
sig = inspect.signature(build_mcp_server)
assert sig.parameters["manager"].kind == inspect.Parameter.KEYWORD_ONLY
def _fake_ctx(session_obj: object) -> Context:
"""Build a Context whose ``session`` attribute returns ``session_obj``.
Context's ``session`` is a property backed by ``request_context.session``;
we construct a minimal ``RequestContext`` and set it as the private
``_request_context`` field. The pydantic public API doesn't expose a
setter for ``session``, so we use ``object.__setattr__`` on the private
backing field.
"""
ctx = Context.model_construct()
request_ctx = RequestContext(
request_id="req-test",
meta=None,
session=session_obj,
lifespan_context=None,
)
object.__setattr__(ctx, "_request_context", request_ctx)
return ctx
def test_current_session_id_is_stable_across_calls_same_session() -> None:
"""Production-path identity: two tool calls from the same MCP session
must yield the same session_id so the busy tracker can renew the lease.
This exercises the ``Context.session`` code path (NOT the
``_TEST_SESSION_ID`` fallback used by ``_call_tool_sync``)."""
sentinel_session = object()
ctx = _fake_ctx(sentinel_session)
first = _current_session_id(ctx)
second = _current_session_id(ctx)
assert first == second
assert first.startswith("mcp_session:")
# Object identity of the underlying ServerSession is the key — verifies
# we use id(ctx.session) rather than e.g. ctx.request_id.
assert first == f"mcp_session:{id(sentinel_session)}"
def test_current_session_id_differs_across_sessions() -> None:
"""Two different MCP sessions (distinct ServerSession objects) must
produce distinct session_ids so the busy tracker can isolate them."""
sess_a = object()
sess_b = object()
assert _current_session_id(_fake_ctx(sess_a)) != _current_session_id(
_fake_ctx(sess_b)
)
def test_current_session_id_falls_back_when_no_context() -> None:
"""When no Context is available (e.g. outside a FastMCP request lifecycle,
or via ``_call_tool_sync`` which omits the ctx kwarg), the test
contextvars override provides the session_id."""
token = None
try:
from host_agent.web import mcp as mcp_mod
token = mcp_mod._TEST_SESSION_ID.set("test-session-xyz")
assert _current_session_id(None) == "test-session-xyz"
finally:
if token is not None:
from host_agent.web import mcp as mcp_mod
mcp_mod._TEST_SESSION_ID.reset(token)
def test_wrapped_tool_accepts_context_kwarg() -> None:
"""The wrapper registered on FastMCP must declare a ``ctx`` parameter so
FastMCP injects the live Context (and ``tool.context_kwarg`` is set to
``"ctx"``). Without this, FastMCP never injects context and we fall
back to the empty test default — the production bug this PR fixes."""
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
tool_manager = server._tool_manager # type: ignore[attr-defined]
tool = tool_manager.get_tool("take_screenshot")
assert tool is not None
assert tool.context_kwarg == "ctx"
def test_busy_error_wire_shape_is_calltoolresult_iserror() -> None:
"""Regression test for spec §7 — busy errors must be visible on the wire.
mcp SDK 1.28.1's ``Tool.run`` wraps every non-``UrlElicitationRequiredError``
exception (including ``McpError`` and our ``McpDeviceBusyError``) into
``ToolError`` (see ``mcp/server/fastmcp/tools/base.py``). The lowlevel
``call_tool`` handler then builds a ``CallToolResult(isError=True,
content=[TextContent(...)])`` (see
``mcp/server/lowlevel/server.py::_make_error_result``). There is no public
path that surfaces JSON-RPC ``-32000`` + structured ``data.busy_owner`` from
a tool call site — the SDK's wire contract for tool errors is the
``isError=true`` flag plus text content. This test pins the wire shape so
any future SDK upgrade that exposes a true JSON-RPC error path is caught."""
import asyncio
import mcp.types as types
from mcp.server.fastmcp import FastMCP
from mcp.server.fastmcp.exceptions import ToolError
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
tracker.acquire("phone-1", "sess-other") # different session holds the device
server: FastMCP = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
tool = server._tool_manager.get_tool("take_screenshot") # type: ignore[attr-defined]
assert tool is not None
sentinel_session = object()
ctx = _fake_ctx(sentinel_session)
with pytest.raises(ToolError) as tool_exc:
asyncio.run(tool.run({"device_id": "phone-1"}, context=ctx))
# ToolError text carries the original exception message verbatim,
# which is what the lowlevel handler copies into TextContent.
message = str(tool_exc.value)
assert "phone-1" in message
assert "busy" in message
assert "mcp_session:sess-oth" in message # truncated busy_owner
# The lowlevel handler converts any exception into a CallToolResult
# with isError=True (mcp SDK 1.28.1 — not a JSON-RPC error envelope).
# We invoke the SDK helper directly to lock the wire contract.
from mcp.server.lowlevel.server import Server as LowlevelServer
lowlevel = LowlevelServer("test-lowlevel")
error_result = lowlevel._make_error_result(message) # type: ignore[attr-defined]
inner = error_result.root
assert isinstance(inner, types.CallToolResult)
assert inner.isError is True
assert len(inner.content) == 1
text_block = inner.content[0]
assert isinstance(text_block, types.TextContent)
assert text_block.text == message
# And confirm the wire shape is NOT a JSON-RPC error envelope — that
# would require code=-32000 + data.busy_owner, which is not exposed
# in mcp SDK 1.28.1 for tool-call errors.
assert not hasattr(inner, "code")
assert inner.structuredContent is None
def test_busy_error_text_includes_cloud_assignment_owner() -> None:
"""Same wire-shape test for the cloud_assignment branch — verifies the
human-readable busy_owner value (the only place to surface it given the
SDK forces tool errors into CallToolResult.isError=true) is correct."""
import asyncio
from mcp.server.fastmcp.exceptions import ToolError
manager = _make_manager_with_device()
tracker = McpBusyTracker()
status = AgentStatusTracker()
status.mark_assignment_started(
AssignmentModel(
task_id="t1",
attempt=1,
lease_id="l1",
lease_expires_at=datetime.now(UTC),
host_id="h1",
device_id="phone-1",
goal="cloud task",
)
)
server = build_mcp_server(
manager=manager, mcp_busy_tracker=tracker, status_tracker=status
)
tool = server._tool_manager.get_tool("take_screenshot") # type: ignore[attr-defined]
assert tool is not None
sentinel_session = object()
ctx = _fake_ctx(sentinel_session)
with pytest.raises(ToolError) as tool_exc:
asyncio.run(tool.run({"device_id": "phone-1"}, context=ctx))
assert "cloud_assignment" in str(tool_exc.value)
assert "phone-1" in str(tool_exc.value)
assert "busy" in str(tool_exc.value)