diff --git a/backend/mcp_server/context.py b/backend/mcp_server/context.py index 20809a77..cfeeb481 100644 --- a/backend/mcp_server/context.py +++ b/backend/mcp_server/context.py @@ -26,15 +26,27 @@ current_client_id: ContextVar[str | None] = ContextVar( "current_client_id", default=None ) +# Endpoints that consume X-Voicebox-Client-Id for its MCP-semantic +# meaning (per-client profile resolution + per-client default_personality). +# These are the paths where a stamp into last_seen_at is accurate. +# Unrelated REST traffic that happens to set the header is intentionally +# ignored so the Settings UI's "last heard from" column only reflects +# calls that actually acted on the client's bindings. +# +# - /mcp — FastMCP tool calls (voicebox.speak, voicebox.transcribe, …) +# and the /mcp/bindings admin surface. The admin surface is never +# called with the header in practice (the frontend manages bindings +# over plain REST), so the `startswith("/mcp")` match doesn't cause +# false stamps. +# - /speak — REST mirror of voicebox.speak for non-MCP agents (shell +# scripts, ACP, A2A). Uses the same per-client binding lookup, so its +# callers belong in the last-seen list too. +_STAMPED_PATH_PREFIXES: tuple[str, ...] = ("/mcp", "/speak") + class ClientIdMiddleware(BaseHTTPMiddleware): - """Copy X-Voicebox-Client-Id into a ContextVar and stamp last_seen_at. - - Only stamps on MCP-endpoint requests (anything under ``/mcp``) so - unrelated REST traffic with the header set won't advance the - last-seen timestamp — the Settings UI uses that to show when each - client was last heard from. - """ + """Copy X-Voicebox-Client-Id into a ContextVar and stamp last_seen_at + for requests that act on the caller's MCP bindings.""" def __init__(self, app: ASGIApp) -> None: super().__init__(app) @@ -47,11 +59,17 @@ class ClientIdMiddleware(BaseHTTPMiddleware): finally: current_client_id.reset(token) - if client_id and request.url.path.startswith("/mcp"): + if client_id and _is_stamped_path(request.url.path): _stamp_last_seen(client_id) return response +def _is_stamped_path(path: str) -> bool: + # Require a path boundary so a future ``/speakers`` or ``/mcpfoo`` + # route doesn't silently inherit the stamp from ``/speak`` / ``/mcp``. + return any(path == p or path.startswith(p + "/") for p in _STAMPED_PATH_PREFIXES) + + def _stamp_last_seen(client_id: str) -> None: """Update or create the MCPClientBinding row for this client_id.""" try: diff --git a/backend/tests/test_client_id_middleware.py b/backend/tests/test_client_id_middleware.py new file mode 100644 index 00000000..c9d081b4 --- /dev/null +++ b/backend/tests/test_client_id_middleware.py @@ -0,0 +1,49 @@ +"""Unit tests for the ClientIdMiddleware path predicate. + +Locks down which endpoints advance ``last_seen_at`` on the +``MCPClientBinding`` row. Getting this wrong is silent: the Settings UI +just shows a stale "last heard from" timestamp and bindings never get +auto-created for new REST callers. +""" + +import pytest + +from backend.mcp_server.context import _is_stamped_path + + +@pytest.mark.parametrize( + "path", + [ + "/mcp", + "/mcp/", + "/mcp/tools/call", + "/mcp/bindings", # admin REST; benign — frontend never sets the header + "/speak", + "/speak/", + ], +) +def test_mcp_semantic_paths_are_stamped(path: str) -> None: + assert _is_stamped_path(path) is True + + +@pytest.mark.parametrize( + "path", + [ + "/", + "/health", + "/generate", + "/captures", + "/profiles", + "/profiles/abc/compose", + "/events/speak", + "/tasks/active", + "/llm/generate", + # Prefix overlap should not match — /speakers is a hypothetical + # future endpoint that shouldn't leak the stamp. + "/speakers", + # Same for anything starting with /mcpfoo. + "/mcpfoo", + ], +) +def test_other_paths_are_not_stamped(path: str) -> None: + assert _is_stamped_path(path) is False