From 083a54721c32e00e8397c87a13a358c36279c3a4 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 13 Oct 2026 18:08:14 +0200 Subject: [PATCH 1/6] feat(mcp): validate and wrap async functions as code tools build_code_tool_spec checks a code tool where it is declared: it must be async, take a ToolContext first, annotate every other parameter, have a description, and carry a name the profile rules accept (now shared through validate_tool_name). register_code_tool hides the context from the advertised schema, checks the read or write scope and any per-tool scope before the function runs, bounds the whole call by the binding's request timeout, and maps exceptions to the MCP error contract. --- redisvl/mcp/config.py | 42 ++- redisvl/mcp/tools/code.py | 194 ++++++++++ tests/unit/test_mcp/test_code_tools_unit.py | 372 ++++++++++++++++++++ 3 files changed, 592 insertions(+), 16 deletions(-) create mode 100644 redisvl/mcp/tools/code.py create mode 100644 tests/unit/test_mcp/test_code_tools_unit.py diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index 244e0298..fd8acbfa 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -57,6 +57,31 @@ def builtin_tool_names() -> frozenset[str]: return _BUILTIN_TOOL_NAMES +def validate_tool_name(name: str, *, kind: str) -> None: + """Raise ``ValueError`` unless ``name`` is usable for a custom tool. + + Shared by every kind of custom tool so that one set of rules decides what a + tool may be called. ``kind`` names the source in the message, so an error + points at the config section or decorator that produced the name. + """ + if not _TOOL_NAME_PATTERN.match(name): + raise ValueError( + f"{kind} name '{name}' is invalid; names must start with a " + "lowercase letter and contain only lowercase letters, digits, " + "hyphens, or underscores (max 64 characters)" + ) + if name in builtin_tool_names(): + raise ValueError( + f"{kind} name '{name}' collides with a built-in tool; " + "built-in names are reserved" + ) + if name.startswith(_RESERVED_TOOL_NAME_PREFIXES): + raise ValueError( + f"{kind} name '{name}' uses a reserved prefix " + f"({', '.join(_RESERVED_TOOL_NAME_PREFIXES)})" + ) + + class MCPRuntimeConfig(BaseModel): """Runtime limits and validated field mappings for MCP requests.""" @@ -765,22 +790,7 @@ def _validate_profile(self) -> "MCPCustomToolConfig": raise ValueError( f"custom_tools '{self.name}' description must not be blank" ) - if not _TOOL_NAME_PATTERN.match(self.name): - raise ValueError( - f"custom_tools name '{self.name}' is invalid; names must start with a " - "lowercase letter and contain only lowercase letters, digits, " - "hyphens, or underscores (max 64 characters)" - ) - if self.name in builtin_tool_names(): - raise ValueError( - f"custom_tools name '{self.name}' collides with a built-in tool; " - "built-in names are reserved" - ) - if self.name.startswith(_RESERVED_TOOL_NAME_PREFIXES): - raise ValueError( - f"custom_tools name '{self.name}' uses a reserved prefix " - f"({', '.join(_RESERVED_TOOL_NAME_PREFIXES)})" - ) + validate_tool_name(self.name, kind="custom_tools") unknown_params = sorted(set(self.params) - _PROFILE_PARAM_NAMES) if unknown_params: diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py new file mode 100644 index 00000000..dab38e1e --- /dev/null +++ b/redisvl/mcp/tools/code.py @@ -0,0 +1,194 @@ +"""Code tools: async Python functions registered as MCP tools. + +A code tool is an async function whose first parameter is a +:class:`~redisvl.mcp.context.ToolContext` and whose remaining, annotated +parameters are the arguments the model supplies. Its docstring (or an explicit +description) is what the model reads when choosing a tool. + +Everything that makes a built-in safe is applied around the function rather +than left to its author: the read or write scope and any per-tool scope are +checked before it runs, the context it receives can only reach Redis through a +guarded, allow-listed handle, the whole call is bounded by the binding's +request timeout, and any exception it raises is mapped to the stable MCP error +contract instead of leaking a traceback. +""" + +import asyncio +import inspect +import typing +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from redisvl.mcp.auth import ensure_read_scope, ensure_tool_scope, ensure_write_scope +from redisvl.mcp.config import validate_tool_name +from redisvl.mcp.context import ToolContext +from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError, map_exception + +_ACCEPTED_KINDS = ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, +) + + +@dataclass(frozen=True) +class CodeToolSpec: + """One validated code tool, ready to register against a binding.""" + + fn: Callable[..., Any] + name: str + description: str + index: str | None + writes: bool + scope: str | None + signature: inspect.Signature + annotations: dict[str, Any] + + +def _qualified(fn: Callable[..., Any]) -> str: + return f"{getattr(fn, '__module__', '?')}.{getattr(fn, '__qualname__', fn)}" + + +def _tool_signature( + fn: Callable[..., Any], +) -> tuple[inspect.Signature, dict[str, Any]]: + """Return the model-facing signature: the function's, minus the context.""" + where = _qualified(fn) + try: + # Resolved rather than read raw, so a module using postponed + # annotations is checked against the real types. + hints = typing.get_type_hints(fn, include_extras=True) + except Exception as exc: + raise TypeError( + f"code tool {where} has annotations that cannot be resolved: {exc}" + ) from exc + + parameters = list(inspect.signature(fn).parameters.values()) + if not parameters or hints.get(parameters[0].name) is not ToolContext: + raise TypeError( + f"code tool {where} must take a ToolContext as its first parameter" + ) + + exposed = [] + annotations: dict[str, Any] = {} + for parameter in parameters[1:]: + if parameter.kind not in _ACCEPTED_KINDS: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' must be a plain " + "named parameter; *args, **kwargs and positional-only " + "parameters cannot be offered to a model" + ) + if parameter.name not in hints: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' needs a type " + "annotation; it is what the tool's input schema is built from" + ) + annotation = hints[parameter.name] + # Keyword-only, because the wrapper is always called with keyword + # arguments decoded from the model's JSON. + exposed.append( + parameter.replace( + kind=inspect.Parameter.KEYWORD_ONLY, annotation=annotation + ) + ) + annotations[parameter.name] = annotation + + return_annotation = hints.get("return", inspect.Signature.empty) + if return_annotation is not inspect.Signature.empty: + annotations["return"] = return_annotation + return ( + inspect.Signature(exposed, return_annotation=return_annotation), + annotations, + ) + + +def build_code_tool_spec( + fn: Callable[..., Any], + *, + name: str | None = None, + namespace: str | None = None, + index: str | None = None, + writes: bool = False, + scope: str | None = None, + description: str | None = None, +) -> CodeToolSpec: + """Validate ``fn`` as a code tool and return its spec. + + Everything checkable without a running server is checked here, so a + malformed tool fails where it is declared rather than at startup. + Whether ``index`` names a real binding, and whether a writing tool is + allowed on it, depends on the server's config and is checked at startup. + """ + where = _qualified(fn) + if not inspect.iscoroutinefunction(fn): + raise TypeError(f"code tool {where} must be an async function") + + signature, annotations = _tool_signature(fn) + + resolved_description = ( + description if description is not None else inspect.getdoc(fn) + ) + if not resolved_description or not resolved_description.strip(): + raise ValueError( + f"code tool {where} needs a description: give it a docstring or pass " + "description=; it is what the model chooses tools by" + ) + + resolved_name = name if name is not None else fn.__name__.replace("_", "-") + if namespace is not None: + resolved_name = f"{namespace}-{resolved_name}" + validate_tool_name(resolved_name, kind="code tool") + + if scope is not None and not scope.strip(): + raise ValueError(f"code tool {where} scope must not be blank") + + return CodeToolSpec( + fn=fn, + name=resolved_name, + description=resolved_description.strip(), + index=index, + writes=writes, + scope=scope, + signature=signature, + annotations=annotations, + ) + + +def register_code_tool(server: Any, spec: CodeToolSpec, binding_id: str) -> None: + """Register one code tool against ``binding_id``.""" + + async def code_tool(**kwargs: Any) -> Any: + # Before the try block, so a refusal reaches the caller as FORBIDDEN + # rather than being remapped. The tool's own scope is required on top + # of the server-wide one, so it can only make a tool stricter. + if spec.writes: + ensure_write_scope(server) + else: + ensure_read_scope(server) + ensure_tool_scope(server, spec.scope) + + try: + runtime = server.resolve_binding(binding_id) + ctx = ToolContext.for_request(server, runtime, writes=spec.writes) + timeout = runtime.binding.runtime.request_timeout_seconds + try: + # The handle bounds each Redis call; this bounds the tool's own + # code too, so a tool cannot hold a request open indefinitely. + return await asyncio.wait_for(spec.fn(ctx, **kwargs), timeout) + except asyncio.TimeoutError as exc: + raise RedisVLMCPError( + f"tool '{spec.name}' did not finish within {timeout:g}s", + code=MCPErrorCode.BACKEND_UNAVAILABLE, + retryable=True, + ) from exc + except RedisVLMCPError: + raise + except Exception as exc: + raise map_exception(exc) from exc + + code_tool.__name__ = spec.name.replace("-", "_") + code_tool.__doc__ = spec.description + code_tool.__signature__ = spec.signature # type: ignore[attr-defined] + code_tool.__annotations__ = dict(spec.annotations) + + server.tool(name=spec.name, description=spec.description)(code_tool) diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py new file mode 100644 index 00000000..cf015b79 --- /dev/null +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -0,0 +1,372 @@ +"""Code tools: what a valid one looks like, and what wraps every call.""" + +import asyncio +from types import SimpleNamespace +from typing import Any + +import pytest +from conftest import _schema + +fastmcp = pytest.importorskip( + "fastmcp", reason="fastmcp not installed (install redisvl[mcp])" +) + +from redisvl.mcp.config import MCPConfig +from redisvl.mcp.context import IndexHandle, ToolContext, WritableIndexHandle +from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError +from redisvl.mcp.runtime import BindingRuntime +from redisvl.mcp.tools.code import build_code_tool_spec, register_code_tool + + +async def recent_tickets(ctx: ToolContext, query: str, limit: int = 5) -> dict: + """Find recent support tickets matching a query.""" + return {"query": query, "limit": limit} + + +class FakeIndex: + async def exists(self): + return True + + +class FakeServer: + def __init__(self, *, timeout: float = 30.0): + self.config = MCPConfig.model_validate( + { + "server": {"redis_url": "redis://localhost:6379"}, + "indexes": { + "tickets": { + "redis_name": "tickets-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + } + }, + } + ) + # Copied rather than validated in, so a test can use a sub-second + # timeout the config model (whole seconds) would refuse. + binding = self.config.indexes["tickets"] + self.binding = binding.model_copy( + update={ + "runtime": binding.runtime.model_copy( + update={"request_timeout_seconds": timeout} + ) + } + ) + self.registered: dict[str, Any] = {} + self.auth_config = SimpleNamespace(read_scope="kb.read", write_scope="kb.write") + self._auth_enabled = False + + def resolve_binding(self, index_id=None): + return BindingRuntime( + binding_id="tickets", + binding=self.binding, + index=FakeIndex(), + schema=_schema(), + vectorizer=None, + supports_native_hybrid_search=False, + effective_read_only=False, + ) + + async def run_guarded(self, operation_name, awaitable, *, timeout_seconds): + return await awaitable + + def tool(self, name=None, description=None, **kwargs): + def decorator(fn): + self.registered[name] = fn + return fn + + return decorator + + +def _registered(fn, server=None, **options): + server = server or FakeServer() + spec = build_code_tool_spec(fn, **options) + register_code_tool(server, spec, "tickets") + return server.registered[spec.name] + + +def _as_caller(monkeypatch, scopes=None): + token = None if scopes is None else SimpleNamespace(scopes=scopes, claims={}) + monkeypatch.setattr( + "fastmcp.server.dependencies.get_access_token", lambda: token, raising=False + ) + + +# -------------------------------------------------------------------------- +# What counts as a code tool +# -------------------------------------------------------------------------- + + +def test_a_sync_function_is_refused(): + def sync_tool(ctx: ToolContext, query: str) -> dict: + """Search.""" + return {} + + with pytest.raises(TypeError, match="must be an async function"): + build_code_tool_spec(sync_tool) + + +async def _no_context(query: str) -> dict: + """Search.""" + return {} + + +async def _untyped_context(ctx, query: str) -> dict: + """Search.""" + return {} + + +async def _unannotated(ctx: ToolContext, query) -> dict: + """Search.""" + return {} + + +async def _varargs(ctx: ToolContext, *queries: str) -> dict: + """Search.""" + return {} + + +async def _varkw(ctx: ToolContext, **options: str) -> dict: + """Search.""" + return {} + + +async def _positional_only(ctx: ToolContext, query: str, /) -> dict: + """Search.""" + return {} + + +async def _unresolvable(ctx: ToolContext, query: "NoSuchType") -> dict: # noqa: F821 + """Search.""" + return {} + + +@pytest.mark.parametrize( + "fn, message", + [ + pytest.param(_no_context, "ToolContext as its first", id="no-context"), + pytest.param(_untyped_context, "ToolContext as its first", id="untyped"), + pytest.param(_unannotated, "'query' needs a type", id="unannotated"), + pytest.param(_varargs, "'queries' must be a plain", id="varargs"), + pytest.param(_varkw, "'options' must be a plain", id="varkw"), + pytest.param(_positional_only, "'query' must be a plain", id="pos-only"), + pytest.param(_unresolvable, "cannot be resolved", id="unresolvable"), + ], +) +def test_a_malformed_signature_is_refused_where_it_is_declared(fn, message): + with pytest.raises(TypeError, match=message): + build_code_tool_spec(fn) + + +def test_string_annotations_are_resolved_to_real_types(): + async def postponed(ctx: "ToolContext", query: "str") -> "dict": + """Search.""" + return {} + + spec = build_code_tool_spec(postponed) + + assert spec.annotations == {"query": str, "return": dict} + + +def test_a_tool_without_a_description_is_refused(): + async def undocumented(ctx: ToolContext, query: str) -> dict: + return {} + + with pytest.raises(ValueError, match="needs a description"): + build_code_tool_spec(undocumented) + with pytest.raises(ValueError, match="needs a description"): + build_code_tool_spec(recent_tickets, description=" ") + + +def test_the_description_is_the_cleaned_docstring_unless_given(): + async def documented(ctx: ToolContext, query: str) -> dict: + """ + Find tickets. + + Use for support questions. + """ + return {} + + assert ( + build_code_tool_spec(documented).description + == "Find tickets.\n\nUse for support questions." + ) + assert ( + build_code_tool_spec(documented, description="Search tickets.").description + == "Search tickets." + ) + + +@pytest.mark.parametrize( + "options, expected", + [ + ({}, "recent-tickets"), + ({"namespace": "support"}, "support-recent-tickets"), + ({"name": "tickets"}, "tickets"), + ({"name": "tickets", "namespace": "support"}, "support-tickets"), + ], +) +def test_the_name_comes_from_the_function_unless_given(options, expected): + assert build_code_tool_spec(recent_tickets, **options).name == expected + + +@pytest.mark.parametrize( + "options, message", + [ + ({"name": "Recent.Tickets"}, "is invalid"), + ({"name": "search-records"}, "collides with a built-in tool"), + ({"namespace": "redisvl"}, "uses a reserved prefix"), + ], +) +def test_a_name_the_profile_rules_refuse_is_refused_here_too(options, message): + with pytest.raises(ValueError, match=f"code tool name .* {message}"): + build_code_tool_spec(recent_tickets, **options) + + +def test_a_blank_scope_is_refused(): + with pytest.raises(ValueError, match="scope must not be blank"): + build_code_tool_spec(recent_tickets, scope=" ") + + +# -------------------------------------------------------------------------- +# What the model sees +# -------------------------------------------------------------------------- + + +async def test_the_model_sees_the_arguments_but_never_the_context(): + mcp = fastmcp.FastMCP("code-tools") + register_code_tool(mcp, build_code_tool_spec(recent_tickets), "tickets") + + async with fastmcp.Client(mcp) as client: + (tool,) = await client.list_tools() + + assert tool.name == "recent-tickets" + assert tool.description == "Find recent support tickets matching a query." + assert set(tool.inputSchema["properties"]) == {"query", "limit"} + assert tool.inputSchema["required"] == ["query"] + + +# -------------------------------------------------------------------------- +# What wraps every call +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "writes, handle_type", [(False, IndexHandle), (True, WritableIndexHandle)] +) +async def test_the_tool_runs_with_a_context_for_its_binding( + monkeypatch, writes, handle_type +): + _as_caller(monkeypatch) + seen: list[ToolContext] = [] + + async def inspect_context(ctx: ToolContext, query: str) -> dict: + """Search.""" + seen.append(ctx) + return {"exists": await ctx.index.exists()} + + fn = _registered(inspect_context, writes=writes) + + assert await fn(query="refund") == {"exists": True} + assert type(seen[0].index) is handle_type + + +@pytest.mark.parametrize( + "writes, scopes", + [ + pytest.param(False, ["kb.write"], id="read-without-read-scope"), + pytest.param(True, ["kb.read"], id="write-without-write-scope"), + pytest.param(False, ["kb.read"], id="read-without-tool-scope"), + pytest.param(True, ["kb.write"], id="write-without-tool-scope"), + ], +) +async def test_a_token_missing_a_required_scope_never_reaches_the_tool( + monkeypatch, writes, scopes +): + called: list[str] = [] + + async def tool_fn(ctx: ToolContext, query: str) -> dict: + """Search.""" + called.append(query) + return {} + + server = FakeServer() + server._auth_enabled = True + _as_caller(monkeypatch, scopes) + fn = _registered(tool_fn, server, writes=writes, scope="tickets.read") + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(query="refund") + + assert exc_info.value.code == MCPErrorCode.FORBIDDEN + assert called == [] + + +@pytest.mark.parametrize( + "writes, scopes", + [(False, ["kb.read", "tickets.read"]), (True, ["kb.write", "tickets.read"])], +) +async def test_the_server_scope_and_the_tool_scope_together_admit_the_caller( + monkeypatch, writes, scopes +): + server = FakeServer() + server._auth_enabled = True + _as_caller(monkeypatch, scopes) + fn = _registered(recent_tickets, server, writes=writes, scope="tickets.read") + + assert await fn(query="refund") == {"query": "refund", "limit": 5} + + +async def test_a_tool_that_overruns_the_binding_timeout_is_cut_off(monkeypatch): + _as_caller(monkeypatch) + + async def slow(ctx: ToolContext, query: str) -> dict: + """Search slowly.""" + await asyncio.sleep(5) + return {} + + fn = _registered(slow, FakeServer(timeout=0.05)) + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(query="refund") + + assert exc_info.value.code == MCPErrorCode.BACKEND_UNAVAILABLE + assert exc_info.value.retryable is True + assert "tool 'slow' did not finish within 0.05s" in str(exc_info.value) + + +@pytest.mark.parametrize( + "raised, code", + [ + (ValueError("bad sku"), MCPErrorCode.INVALID_REQUEST), + (RuntimeError("boom"), MCPErrorCode.INTERNAL_ERROR), + ], +) +async def test_an_exception_from_the_tool_is_mapped(monkeypatch, raised, code): + _as_caller(monkeypatch) + + async def failing(ctx: ToolContext, query: str) -> dict: + """Search.""" + raise raised + + with pytest.raises(RedisVLMCPError) as exc_info: + await _registered(failing)(query="refund") + + assert exc_info.value.code == code + assert exc_info.value.__cause__ is raised + + +async def test_an_mcp_error_from_the_tool_passes_through_unchanged(monkeypatch): + _as_caller(monkeypatch) + error = RedisVLMCPError( + "sku not found", code=MCPErrorCode.INVALID_REQUEST, retryable=False + ) + + async def failing(ctx: ToolContext, query: str) -> dict: + """Search.""" + raise error + + with pytest.raises(RedisVLMCPError) as exc_info: + await _registered(failing)(query="refund") + + assert exc_info.value is error From 9717e52ee0fadf14b23f03d5680b2655896c7d62 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 14 Oct 2026 10:18:10 +0200 Subject: [PATCH 2/6] fix(mcp): refuse a keyword-only context and re-check read-only per call A ToolContext declared keyword-only passed validation and then failed every call, because the wrapper passes it positionally; it is now refused where it is declared. A writing tool re-checks that its binding is still writable on each call, since tools register once while a restart re-reads the config. Name errors now point at the function, and the wrapper's tests cover the binding it resolves, a call over the wire and a timed-out call releasing its concurrency slot. --- redisvl/mcp/tools/code.py | 32 +++++-- tests/unit/test_mcp/test_code_tools_unit.py | 99 +++++++++++++++++++-- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py index dab38e1e..214cfc06 100644 --- a/redisvl/mcp/tools/code.py +++ b/redisvl/mcp/tools/code.py @@ -64,9 +64,20 @@ def _tool_signature( ) from exc parameters = list(inspect.signature(fn).parameters.values()) - if not parameters or hints.get(parameters[0].name) is not ToolContext: + # Positional, because the wrapper passes the context positionally; a + # keyword-only one would be accepted here and fail on every call. + if ( + not parameters + or hints.get(parameters[0].name) is not ToolContext + or parameters[0].kind + not in ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ) + ): raise TypeError( - f"code tool {where} must take a ToolContext as its first parameter" + f"code tool {where} must take a ToolContext as its first, positional " + "parameter" ) exposed = [] @@ -137,7 +148,7 @@ def build_code_tool_spec( resolved_name = name if name is not None else fn.__name__.replace("_", "-") if namespace is not None: resolved_name = f"{namespace}-{resolved_name}" - validate_tool_name(resolved_name, kind="code tool") + validate_tool_name(resolved_name, kind=f"code tool {where}") if scope is not None and not scope.strip(): raise ValueError(f"code tool {where} scope must not be blank") @@ -158,9 +169,9 @@ def register_code_tool(server: Any, spec: CodeToolSpec, binding_id: str) -> None """Register one code tool against ``binding_id``.""" async def code_tool(**kwargs: Any) -> Any: - # Before the try block, so a refusal reaches the caller as FORBIDDEN - # rather than being remapped. The tool's own scope is required on top - # of the server-wide one, so it can only make a tool stricter. + # Before anything else, so a caller without the scope never reaches the + # binding or the function. The tool's own scope is required on top of + # the server-wide one, so it can only make a tool stricter. if spec.writes: ensure_write_scope(server) else: @@ -169,6 +180,15 @@ async def code_tool(**kwargs: Any) -> Any: try: runtime = server.resolve_binding(binding_id) + if spec.writes and runtime.effective_read_only: + # Startup refuses this pairing, but the binding is re-read on + # every start while tools register once, so a restart against + # a config that made the index read-only lands here. + raise RedisVLMCPError( + f"index '{binding_id}' is read-only", + code=MCPErrorCode.FORBIDDEN, + retryable=False, + ) ctx = ToolContext.for_request(server, runtime, writes=spec.writes) timeout = runtime.binding.runtime.request_timeout_seconds try: diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py index cf015b79..fb291402 100644 --- a/tests/unit/test_mcp/test_code_tools_unit.py +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -1,6 +1,7 @@ """Code tools: what a valid one looks like, and what wraps every call.""" import asyncio +from dataclasses import replace from types import SimpleNamespace from typing import Any @@ -15,6 +16,8 @@ from redisvl.mcp.context import IndexHandle, ToolContext, WritableIndexHandle from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.runtime import BindingRuntime +from redisvl.mcp.server import RedisVLMCPServer +from redisvl.mcp.settings import MCPSettings from redisvl.mcp.tools.code import build_code_tool_spec, register_code_tool @@ -29,7 +32,7 @@ async def exists(self): class FakeServer: - def __init__(self, *, timeout: float = 30.0): + def __init__(self, *, timeout: float = 30.0, read_only: bool = False): self.config = MCPConfig.model_validate( { "server": {"redis_url": "redis://localhost:6379"}, @@ -52,11 +55,14 @@ def __init__(self, *, timeout: float = 30.0): ) } ) + self.read_only = read_only + self.resolved: list[str | None] = [] self.registered: dict[str, Any] = {} self.auth_config = SimpleNamespace(read_scope="kb.read", write_scope="kb.write") self._auth_enabled = False def resolve_binding(self, index_id=None): + self.resolved.append(index_id) return BindingRuntime( binding_id="tickets", binding=self.binding, @@ -64,7 +70,7 @@ def resolve_binding(self, index_id=None): schema=_schema(), vectorizer=None, supports_native_hybrid_search=False, - effective_read_only=False, + effective_read_only=self.read_only, ) async def run_guarded(self, operation_name, awaitable, *, timeout_seconds): @@ -116,6 +122,11 @@ async def _untyped_context(ctx, query: str) -> dict: return {} +async def _keyword_only_context(*, ctx: ToolContext, query: str) -> dict: + """Search.""" + return {} + + async def _unannotated(ctx: ToolContext, query) -> dict: """Search.""" return {} @@ -146,6 +157,9 @@ async def _unresolvable(ctx: ToolContext, query: "NoSuchType") -> dict: # noqa: [ pytest.param(_no_context, "ToolContext as its first", id="no-context"), pytest.param(_untyped_context, "ToolContext as its first", id="untyped"), + pytest.param( + _keyword_only_context, "first, positional parameter", id="kw-only-context" + ), pytest.param(_unannotated, "'query' needs a type", id="unannotated"), pytest.param(_varargs, "'queries' must be a plain", id="varargs"), pytest.param(_varkw, "'options' must be a plain", id="varkw"), @@ -214,12 +228,14 @@ def test_the_name_comes_from_the_function_unless_given(options, expected): "options, message", [ ({"name": "Recent.Tickets"}, "is invalid"), - ({"name": "search-records"}, "collides with a built-in tool"), + # The namespace is applied before the name is checked. ({"namespace": "redisvl"}, "uses a reserved prefix"), ], ) def test_a_name_the_profile_rules_refuse_is_refused_here_too(options, message): - with pytest.raises(ValueError, match=f"code tool name .* {message}"): + with pytest.raises( + ValueError, match=f"code tool .*recent_tickets name .* {message}" + ): build_code_tool_spec(recent_tickets, **options) @@ -233,17 +249,29 @@ def test_a_blank_scope_is_refused(): # -------------------------------------------------------------------------- -async def test_the_model_sees_the_arguments_but_never_the_context(): +async def test_the_model_sees_the_arguments_but_never_the_context(monkeypatch): + # A real FastMCP server and client, so the call below goes through FastMCP's + # own argument validation against the synthesised signature. + _as_caller(monkeypatch) + fake = FakeServer() mcp = fastmcp.FastMCP("code-tools") + mcp.resolve_binding = fake.resolve_binding + mcp.run_guarded = fake.run_guarded register_code_tool(mcp, build_code_tool_spec(recent_tickets), "tickets") async with fastmcp.Client(mcp) as client: (tool,) = await client.list_tools() + called = await client.call_tool("recent-tickets", {"query": "refund"}) + refused = await client.call_tool( + "recent-tickets", {"query": "refund", "ctx": {}}, raise_on_error=False + ) assert tool.name == "recent-tickets" assert tool.description == "Find recent support tickets matching a query." assert set(tool.inputSchema["properties"]) == {"query", "limit"} assert tool.inputSchema["required"] == ["query"] + assert called.data == {"query": "refund", "limit": 5} + assert refused.is_error # -------------------------------------------------------------------------- @@ -265,12 +293,34 @@ async def inspect_context(ctx: ToolContext, query: str) -> dict: seen.append(ctx) return {"exists": await ctx.index.exists()} - fn = _registered(inspect_context, writes=writes) + server = FakeServer() + fn = _registered(inspect_context, server, writes=writes) assert await fn(query="refund") == {"exists": True} + assert server.resolved == ["tickets"] assert type(seen[0].index) is handle_type +async def test_a_writing_tool_is_refused_on_an_index_now_read_only(monkeypatch): + # Startup refuses the pairing, but tools register once and the binding is + # re-read on every start, so the call itself has to check too. + _as_caller(monkeypatch) + called: list[str] = [] + + async def write(ctx: ToolContext, ticket_id: str) -> dict: + """Close a ticket.""" + called.append(ticket_id) + return {} + + fn = _registered(write, FakeServer(read_only=True), writes=True) + + with pytest.raises(RedisVLMCPError) as exc_info: + await fn(ticket_id="t1") + + assert exc_info.value.code == MCPErrorCode.FORBIDDEN + assert called == [] + + @pytest.mark.parametrize( "writes, scopes", [ @@ -370,3 +420,40 @@ async def failing(ctx: ToolContext, query: str) -> dict: await _registered(failing)(query="refund") assert exc_info.value is error + + +async def test_a_timed_out_call_gives_back_its_concurrency_slot(monkeypatch): + # Against the real run_guarded: a tool cut off mid-call must release the + # semaphore and the active-request count, or the cap leaks one slot per + # timeout until the server stops answering. + _as_caller(monkeypatch) + + class HangingIndex: + async def exists(self): + await asyncio.sleep(5) + + server = RedisVLMCPServer(MCPSettings(config="/tmp/mcp.yaml")) + server._semaphore = asyncio.Semaphore(1) + server._lifecycle_state = server._LifecycleState.RUNNING + server.config = FakeServer().config + fake = FakeServer(timeout=0.05) + runtime = fake.resolve_binding("tickets") + server._bindings = {"tickets": replace(runtime, index=HangingIndex())} + registered: dict[str, Any] = {} + monkeypatch.setattr( + server, + "tool", + lambda name=None, description=None: lambda fn: registered.setdefault(name, fn), + ) + + async def probe(ctx: ToolContext) -> dict: + """Probe the index.""" + return {"exists": await ctx.index.exists()} + + register_code_tool(server, build_code_tool_spec(probe), "tickets") + + with pytest.raises(RedisVLMCPError): + await registered["probe"]() + + assert server._active_requests == 0 + assert server._semaphore.locked() is False From 9977626459aaedea506d88095f55e1397842da82 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 20 Oct 2026 11:48:24 +0200 Subject: [PATCH 3/6] fix(mcp): check a code tool's input schema where it is declared, and close a queued call on timeout --- redisvl/mcp/server.py | 12 ++++++- redisvl/mcp/tools/code.py | 17 ++++++++-- tests/unit/test_mcp/test_code_tools_unit.py | 35 ++++++++++++++++----- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 69c83abc..ab550318 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -266,7 +266,15 @@ async def run_guarded( self._close_awaitable(awaitable) raise RuntimeError("MCP server is not running") - async with semaphore: + try: + await semaphore.acquire() + except BaseException: + # Cancelled while waiting for a slot, as by a code tool's whole-call + # timeout, so the awaitable never started. + self._close_awaitable(awaitable) + raise + + try: async with self._request_state_lock: if self._lifecycle_state is not _LifecycleState.RUNNING: self._close_awaitable(awaitable) @@ -286,6 +294,8 @@ async def run_guarded( self._active_requests -= 1 if self._active_requests == 0: self._active_requests_drained.set() + finally: + semaphore.release() @staticmethod def _build_vectorizer(binding: MCPIndexBindingConfig) -> Any: diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py index 214cfc06..3b49d46d 100644 --- a/redisvl/mcp/tools/code.py +++ b/redisvl/mcp/tools/code.py @@ -20,6 +20,8 @@ from dataclasses import dataclass from typing import Any +from pydantic import TypeAdapter + from redisvl.mcp.auth import ensure_read_scope, ensure_tool_scope, ensure_write_scope from redisvl.mcp.config import validate_tool_name from redisvl.mcp.context import ToolContext @@ -95,6 +97,15 @@ def _tool_signature( "annotation; it is what the tool's input schema is built from" ) annotation = hints[parameter.name] + try: + # Built here so a type the input schema cannot express fails at + # declaration, not at startup after every binding has connected. + TypeAdapter(annotation).json_schema() + except Exception as exc: + raise TypeError( + f"code tool {where} parameter '{parameter.name}' has a type " + f"that no input schema can describe: {exc}" + ) from exc # Keyword-only, because the wrapper is always called with keyword # arguments decoded from the model's JSON. exposed.append( @@ -181,9 +192,9 @@ async def code_tool(**kwargs: Any) -> Any: try: runtime = server.resolve_binding(binding_id) if spec.writes and runtime.effective_read_only: - # Startup refuses this pairing, but the binding is re-read on - # every start while tools register once, so a restart against - # a config that made the index read-only lands here. + # Startup already refuses this pairing; checked again per call, + # as upsert_records does, so a write never depends on that + # check alone. raise RedisVLMCPError( f"index '{binding_id}' is read-only", code=MCPErrorCode.FORBIDDEN, diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py index fb291402..43edcf7d 100644 --- a/tests/unit/test_mcp/test_code_tools_unit.py +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -1,6 +1,7 @@ """Code tools: what a valid one looks like, and what wraps every call.""" import asyncio +import inspect from dataclasses import replace from types import SimpleNamespace from typing import Any @@ -152,6 +153,15 @@ async def _unresolvable(ctx: ToolContext, query: "NoSuchType") -> dict: # noqa: return {} +class _NoSchema: + pass + + +async def _no_schema(ctx: ToolContext, query: _NoSchema) -> dict: + """Search.""" + return {} + + @pytest.mark.parametrize( "fn, message", [ @@ -165,6 +175,7 @@ async def _unresolvable(ctx: ToolContext, query: "NoSuchType") -> dict: # noqa: pytest.param(_varkw, "'options' must be a plain", id="varkw"), pytest.param(_positional_only, "'query' must be a plain", id="pos-only"), pytest.param(_unresolvable, "cannot be resolved", id="unresolvable"), + pytest.param(_no_schema, "no input schema can describe", id="no-schema"), ], ) def test_a_malformed_signature_is_refused_where_it_is_declared(fn, message): @@ -302,8 +313,7 @@ async def inspect_context(ctx: ToolContext, query: str) -> dict: async def test_a_writing_tool_is_refused_on_an_index_now_read_only(monkeypatch): - # Startup refuses the pairing, but tools register once and the binding is - # re-read on every start, so the call itself has to check too. + # Startup refuses the pairing too; the call checks again on its own. _as_caller(monkeypatch) called: list[str] = [] @@ -324,8 +334,8 @@ async def write(ctx: ToolContext, ticket_id: str) -> dict: @pytest.mark.parametrize( "writes, scopes", [ - pytest.param(False, ["kb.write"], id="read-without-read-scope"), - pytest.param(True, ["kb.read"], id="write-without-write-scope"), + pytest.param(False, ["kb.write", "tickets.read"], id="read-without-read-scope"), + pytest.param(True, ["kb.read", "tickets.read"], id="write-without-write-scope"), pytest.param(False, ["kb.read"], id="read-without-tool-scope"), pytest.param(True, ["kb.write"], id="write-without-tool-scope"), ], @@ -422,15 +432,19 @@ async def failing(ctx: ToolContext, query: str) -> dict: assert exc_info.value is error -async def test_a_timed_out_call_gives_back_its_concurrency_slot(monkeypatch): +@pytest.mark.parametrize("slot_free", [True, False], ids=["mid-call", "queued"]) +async def test_a_timed_out_call_gives_back_its_concurrency_slot(monkeypatch, slot_free): # Against the real run_guarded: a tool cut off mid-call must release the # semaphore and the active-request count, or the cap leaks one slot per - # timeout until the server stops answering. + # timeout until the server stops answering. One cut off while queued for + # the slot must close the Redis call it never started. _as_caller(monkeypatch) + started: list[Any] = [] class HangingIndex: - async def exists(self): - await asyncio.sleep(5) + def exists(self): + started.append(asyncio.sleep(5)) + return started[-1] server = RedisVLMCPServer(MCPSettings(config="/tmp/mcp.yaml")) server._semaphore = asyncio.Semaphore(1) @@ -451,9 +465,14 @@ async def probe(ctx: ToolContext) -> dict: return {"exists": await ctx.index.exists()} register_code_tool(server, build_code_tool_spec(probe), "tickets") + if not slot_free: + await server._semaphore.acquire() with pytest.raises(RedisVLMCPError): await registered["probe"]() assert server._active_requests == 0 + assert inspect.getcoroutinestate(started[0]) == inspect.CORO_CLOSED + if not slot_free: + server._semaphore.release() assert server._semaphore.locked() is False From 74978d44a6c2ad577a6ee783e18468f184446f9d Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 29 Oct 2026 14:53:00 +0100 Subject: [PATCH 4/6] fix(mcp): require the read scope for writing code tools, and match tool names in full --- redisvl/mcp/config.py | 3 ++- redisvl/mcp/tools/code.py | 6 +++--- tests/unit/test_mcp/test_code_tools_unit.py | 11 ++++++++--- tests/unit/test_mcp/test_config.py | 2 ++ 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index fd8acbfa..562bbb96 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -64,7 +64,8 @@ def validate_tool_name(name: str, *, kind: str) -> None: tool may be called. ``kind`` names the source in the message, so an error points at the config section or decorator that produced the name. """ - if not _TOOL_NAME_PATTERN.match(name): + # fullmatch, because `$` also matches before a trailing newline. + if not _TOOL_NAME_PATTERN.fullmatch(name): raise ValueError( f"{kind} name '{name}' is invalid; names must start with a " "lowercase letter and contain only lowercase letters, digits, " diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py index 3b49d46d..919e4dad 100644 --- a/redisvl/mcp/tools/code.py +++ b/redisvl/mcp/tools/code.py @@ -182,11 +182,11 @@ def register_code_tool(server: Any, spec: CodeToolSpec, binding_id: str) -> None async def code_tool(**kwargs: Any) -> Any: # Before anything else, so a caller without the scope never reaches the # binding or the function. The tool's own scope is required on top of - # the server-wide one, so it can only make a tool stricter. + # the server-wide one, so it can only make a tool stricter. A writing + # tool needs the read scope too, because its handle can also read. + ensure_read_scope(server) if spec.writes: ensure_write_scope(server) - else: - ensure_read_scope(server) ensure_tool_scope(server, spec.scope) try: diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py index 43edcf7d..fbecd1e3 100644 --- a/tests/unit/test_mcp/test_code_tools_unit.py +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -239,13 +239,14 @@ def test_the_name_comes_from_the_function_unless_given(options, expected): "options, message", [ ({"name": "Recent.Tickets"}, "is invalid"), + ({"name": "recent-tickets\n"}, "is invalid"), # The namespace is applied before the name is checked. ({"namespace": "redisvl"}, "uses a reserved prefix"), ], ) def test_a_name_the_profile_rules_refuse_is_refused_here_too(options, message): with pytest.raises( - ValueError, match=f"code tool .*recent_tickets name .* {message}" + ValueError, match=f"(?s)code tool .*recent_tickets name .* {message}" ): build_code_tool_spec(recent_tickets, **options) @@ -336,8 +337,9 @@ async def write(ctx: ToolContext, ticket_id: str) -> dict: [ pytest.param(False, ["kb.write", "tickets.read"], id="read-without-read-scope"), pytest.param(True, ["kb.read", "tickets.read"], id="write-without-write-scope"), + pytest.param(True, ["kb.write", "tickets.read"], id="write-without-read-scope"), pytest.param(False, ["kb.read"], id="read-without-tool-scope"), - pytest.param(True, ["kb.write"], id="write-without-tool-scope"), + pytest.param(True, ["kb.read", "kb.write"], id="write-without-tool-scope"), ], ) async def test_a_token_missing_a_required_scope_never_reaches_the_tool( @@ -364,7 +366,10 @@ async def tool_fn(ctx: ToolContext, query: str) -> dict: @pytest.mark.parametrize( "writes, scopes", - [(False, ["kb.read", "tickets.read"]), (True, ["kb.write", "tickets.read"])], + [ + (False, ["kb.read", "tickets.read"]), + (True, ["kb.read", "kb.write", "tickets.read"]), + ], ) async def test_the_server_scope_and_the_tool_scope_together_admit_the_caller( monkeypatch, writes, scopes diff --git a/tests/unit/test_mcp/test_config.py b/tests/unit/test_mcp/test_config.py index a16267f9..4f76c77a 100644 --- a/tests/unit/test_mcp/test_config.py +++ b/tests/unit/test_mcp/test_config.py @@ -693,6 +693,8 @@ def test_mcp_config_custom_tool_defaults(): # character, and the body character class. "1foo", "shop.foo", + # `$` alone would accept this. + "foo\n", ], ) def test_mcp_config_rejects_invalid_custom_tool_names(name): From a1479425fad48151a76f1a988d575aaa6126ed25 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 13:17:33 +0100 Subject: [PATCH 5/6] test(mcp): bound the queued-timeout test so a lost timeout fails instead of hanging --- tests/unit/test_mcp/test_code_tools_unit.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py index fbecd1e3..bcc8179f 100644 --- a/tests/unit/test_mcp/test_code_tools_unit.py +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -474,7 +474,9 @@ async def probe(ctx: ToolContext) -> dict: await server._semaphore.acquire() with pytest.raises(RedisVLMCPError): - await registered["probe"]() + # Bounded here too, so a lost whole-call timeout fails the test + # instead of leaving it queued forever behind the slot held above. + await asyncio.wait_for(registered["probe"](), 2) assert server._active_requests == 0 assert inspect.getcoroutinestate(started[0]) == inspect.CORO_CLOSED From ecf774a2b613fcd0f449f50dfc9d62d7df25bd35 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 14:32:24 +0100 Subject: [PATCH 6/6] fix(mcp): refuse a code tool scope with surrounding whitespace --- redisvl/mcp/tools/code.py | 5 +++++ tests/unit/test_mcp/test_code_tools_unit.py | 14 +++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py index 919e4dad..2c3bba83 100644 --- a/redisvl/mcp/tools/code.py +++ b/redisvl/mcp/tools/code.py @@ -163,6 +163,11 @@ def build_code_tool_spec( if scope is not None and not scope.strip(): raise ValueError(f"code tool {where} scope must not be blank") + if scope is not None and scope != scope.strip(): + # Scopes are matched exactly, so a padded one would refuse every caller. + raise ValueError( + f"code tool {where} scope {scope!r} has surrounding whitespace" + ) return CodeToolSpec( fn=fn, diff --git a/tests/unit/test_mcp/test_code_tools_unit.py b/tests/unit/test_mcp/test_code_tools_unit.py index bcc8179f..6cbd01aa 100644 --- a/tests/unit/test_mcp/test_code_tools_unit.py +++ b/tests/unit/test_mcp/test_code_tools_unit.py @@ -251,9 +251,17 @@ def test_a_name_the_profile_rules_refuse_is_refused_here_too(options, message): build_code_tool_spec(recent_tickets, **options) -def test_a_blank_scope_is_refused(): - with pytest.raises(ValueError, match="scope must not be blank"): - build_code_tool_spec(recent_tickets, scope=" ") +@pytest.mark.parametrize( + "scope, message", + [ + (" ", "scope must not be blank"), + # Scopes match exactly, so this one would refuse every caller. + (" tickets.read", "has surrounding whitespace"), + ], +) +def test_an_unusable_scope_is_refused(scope, message): + with pytest.raises(ValueError, match=message): + build_code_tool_spec(recent_tickets, scope=scope) # --------------------------------------------------------------------------