mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-10-03 17:15:19 -07:00
fix(mcp): stamp last_seen_at on /speak too + tighten path predicate
POST /speak is a REST wrapper around voicebox.speak for agents that
don't talk MCP (shell scripts, ACP, A2A). It reads X-Voicebox-Client-Id
and uses it for the same per-client profile resolution + default
personality lookup the MCP tool does (speak.py:39-64), so its callers
are first-class clients — but the ClientIdMiddleware only stamped
last_seen_at on /mcp* paths. REST speak callers showed up as "never
seen" in Settings → MCP despite actively acting on their bindings.
Widen the stamp predicate to an explicit ("/mcp", "/speak") prefix
list, and require a path boundary on match so future routes named
/mcpfoo or /speakers don't silently inherit the stamp via the prefix.
New test_client_id_middleware.py pins the scope with 17 parametrised
cases (both the allowed set and the overlap cases that must not match).
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
30c2cf1a2c
commit
c9103a24da
@@ -26,15 +26,27 @@ current_client_id: ContextVar[str | None] = ContextVar(
|
|||||||
"current_client_id", default=None
|
"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):
|
class ClientIdMiddleware(BaseHTTPMiddleware):
|
||||||
"""Copy X-Voicebox-Client-Id into a ContextVar and stamp last_seen_at.
|
"""Copy X-Voicebox-Client-Id into a ContextVar and stamp last_seen_at
|
||||||
|
for requests that act on the caller's MCP bindings."""
|
||||||
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.
|
|
||||||
"""
|
|
||||||
|
|
||||||
def __init__(self, app: ASGIApp) -> None:
|
def __init__(self, app: ASGIApp) -> None:
|
||||||
super().__init__(app)
|
super().__init__(app)
|
||||||
@@ -47,11 +59,17 @@ class ClientIdMiddleware(BaseHTTPMiddleware):
|
|||||||
finally:
|
finally:
|
||||||
current_client_id.reset(token)
|
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)
|
_stamp_last_seen(client_id)
|
||||||
return response
|
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:
|
def _stamp_last_seen(client_id: str) -> None:
|
||||||
"""Update or create the MCPClientBinding row for this client_id."""
|
"""Update or create the MCPClientBinding row for this client_id."""
|
||||||
try:
|
try:
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user