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
parent e69cea0245
commit 70e0624a47
8 changed files with 207 additions and 35 deletions
@@ -310,3 +310,106 @@ def test_wrapped_tool_accepts_context_kwarg() -> None:
    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)