From 083a54721c32e00e8397c87a13a358c36279c3a4 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 13 Oct 2026 18:08:14 +0200 Subject: [PATCH 01/15] 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 02/15] 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 03/15] 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 04/15] 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 05/15] 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 06/15] 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) # -------------------------------------------------------------------------- From df428265413d00358c6fa27a1979d262bce10dd6 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 13 Oct 2026 18:16:16 +0200 Subject: [PATCH 07/15] feat(mcp): register code tools from Python with server.custom_tool server.custom_tool(...) registers an async function as an MCP tool on a server started from the caller's own process. At startup each tool is paired with its binding and refused when it cannot hold: a missing or unknown index, a name another custom tool uses, writes on a read-only binding or server, or any binding over a Redis index that a profile scopes with lock.inject, since nothing shows a code tool applies that scope. Code tools register after profiles and count towards the tool-surface fingerprint. ToolContext is now exported from redisvl.mcp, and the concepts and how-to guides document both. --- docs/concepts/mcp.md | 55 +++- docs/user_guide/how_to_guides/mcp.md | 59 ++++- redisvl/mcp/__init__.py | 2 + redisvl/mcp/server.py | 143 ++++++++++- tests/integration/test_mcp/test_code_tools.py | 224 ++++++++++++++++ tests/unit/test_mcp/test_server_unit.py | 241 ++++++++++++++++++ 6 files changed, 715 insertions(+), 9 deletions(-) create mode 100644 tests/integration/test_mcp/test_code_tools.py diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index d0c03489..7043feff 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -158,7 +158,7 @@ The discovery payload is deliberately minimal: The built-in tools expose the index generically, which leaves the model doing query engineering on every call: pick the index, understand the schema, build a filter, choose return fields. A **profile** moves those decisions into config. It is `search-records` with some arguments pre-filled and frozen and the rest still exposed, published under a name and description of your choosing. -Profiles are pure configuration. You add a `custom_tools` entry to the same YAML the server already loads and restart it; there is no Python to write. +Profiles are pure configuration. You add a `custom_tools` entry to the same YAML the server already loads and restart it; a profile needs no Python. For logic a profile cannot express, write a [code tool](#code-tools) instead. ```yaml custom_tools: @@ -243,7 +243,7 @@ Injection is checked at startup wherever the configuration alone can show it wou - authentication is not enabled, on any transport, including an unauthenticated loopback HTTP bind and any `--allow-unauthenticated` bind; - authentication is configured but the server runs over `stdio`, which is never authenticated (checked when the server starts through `rvl mcp` or `run_async`; an embedder that calls `startup()` directly is not, and every call is then refused at request time instead); -- the Redis index is also reachable without the tenant scope: through `search-records`, through `upsert-records` unless every binding over it is read-only, or through another custom tool on it that does not inject exactly the same entries, since a tool scoped by another field, or by the same field from another claim, reads across the tenants this one separates. Bindings that share a `redis_name` count as one index; +- the Redis index is also reachable without the tenant scope: through `search-records`, through `upsert-records` unless every binding over it is read-only, through another custom tool on it that does not inject exactly the same entries, since a tool scoped by another field, or by the same field from another claim, reads across the tenants this one separates, or through a [code tool](#code-tools) bound to it, since the server cannot see what a code tool queries. Bindings that share a `redis_name` count as one index; - the injected field is absent from the bound index, is not a tag field, or is declared `NOINDEX`; - an `inject` list is empty, names one field twice, or names a field that `lock.filter` also constrains; - `required` is anything but `true`, or `from` is anything but `claim`. @@ -267,6 +267,57 @@ Where tenants share one index, the injected filter is the only isolation boundar Listing the tenant claim under `auth.required_claims` is a cheap outer layer: the verifier then rejects a token that lacks the claim before any tool runs. That check confirms only that the claim is present. Its value is still validated by the profile on every call. +## Code Tools + +A **code tool** is an async Python function registered as an MCP tool. It is for logic a profile cannot express, such as re-ranking results, counting matches, or combining several queries into one answer. You register it on the server object from your own Python process: + +```python +from redisvl.mcp import MCPSettings, RedisVLMCPServer, ToolContext +from redisvl.query import CountQuery +from redisvl.query.filter import Tag + +server = RedisVLMCPServer(MCPSettings(config="mcp.yaml")) + + +@server.custom_tool(index="tickets") +async def count_open_tickets(ctx: ToolContext, product: str) -> dict: + """Count open support tickets for a product.""" + open_for_product = (Tag("status") == "open") & (Tag("product") == product) + return {"open": await ctx.index.query(CountQuery(open_for_product))} +``` + +The function's first parameter is the context and the rest are the tool's arguments, which the model supplies. The model sees the arguments and their types, and never the context. The docstring is the description the model chooses tools by, so a tool without one is refused. The tool's name is the function's name with hyphens for underscores (`count-open-tickets`), or `name=`; `namespace=` prefixes it. The same naming rules as profiles apply. + +`index` pins the binding the tool works on. It may be omitted only when exactly one index is configured, and the model never chooses it. + +### What a Code Tool Can Reach + +A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis client. It receives a `ToolContext` with: + +| Attribute | What it is | +|---|---| +| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_keys`, `drop_documents` and `expire_keys`. | +| `ctx.schema` | The bound index's schema. | +| `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | +| `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the request has no token. | + +Everything else on the index is out of reach: creating, dropping or clearing it, `info` for another index, `paginate`, and the filter-wide bulk writes `update_by_filter` and `drop_by_filter`, which a request timeout could cancel partway through. + +The server applies the same rails as for a built-in, without the author asking for them: + +- Every `ctx.index` call takes a slot under the server-wide concurrency cap and is bounded by the binding's `runtime.request_timeout_seconds`. The cap is taken per call, so a tool that queries several times never blocks itself. +- The whole tool call is bounded by the same timeout, so a tool cannot hold a request open indefinitely. A timeout returns a retryable `backend_unavailable` error. +- A caller needs the server's read scope, or the write scope for a tool declared with `writes=True`. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. +- An exception the tool raises becomes a stable MCP error: `ValueError` becomes `invalid_request`, a Redis error a retryable `backend_unavailable`, most other exceptions `internal_error`. Raise `RedisVLMCPError` to choose the code and message yourself. + +The handle is a guardrail, not a sandbox. A code tool is your own code, running with the privileges of the process that started the server, and Python can still reach private attributes. What the handle does is make the bounded, policy-checked path the only obvious one. + +### What Fails for a Code Tool + +The function is checked when it is decorated: it must be async, take a `ToolContext` first, annotate every other parameter, and have a description. Registering a tool after the server has started is refused, because tools register once per process. + +The rest is checked at startup, against the loaded config: an `index` that is missing with several indexes configured or that names no binding; a name another custom tool already uses; a tool declared with `writes=True` on a read-only binding or server; and a code tool on a Redis index that a profile scopes with `lock.inject`. A code tool can run any query its handle allows, so nothing would show that it applies the tenant scope. Bind it to another index, or serve it from a separate server. + ## Why Use MCP Instead of Direct RedisVL Calls Use RedisVL MCP when you want a standard tool boundary for agent frameworks or assistants that already speak MCP. diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 416a59db..7487e11f 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -407,7 +407,7 @@ What the client sees for `search-customer-kb`: - A description whose field hints list `content` but not `org_id`. - Results from its own tenant only. A `filter` naming `org_id` ANDs with the injected value, so naming another tenant returns nothing. Results still carry `org_id`, always with the caller's own value, unless you lock `return_fields` to leave it out. -The server refuses to start while anything else can reach the same Redis index without the tenant scope: `search-records`, `upsert-records` unless every binding over the index is `read_only`, or another custom tool on that index whose `lock.inject` differs, including one with none. Bindings that share a `redis_name` are the same index, so a vector tool and a fulltext tool over one set of documents must inject the same scope. Each of those routes would hand every caller a way round the profile, and a write could retag another tenant's document as the writer's own. Ingest documents outside the server, stamping `org_id` exactly as the identity provider emits it. +The server refuses to start while anything else can reach the same Redis index without the tenant scope: `search-records`, `upsert-records` unless every binding over the index is `read_only`, another custom tool on that index whose `lock.inject` differs, including one with none, or any code tool bound to it. Bindings that share a `redis_name` are the same index, so a vector tool and a fulltext tool over one set of documents must inject the same scope. Each of those routes would hand every caller a way round the profile, and a write could retag another tenant's document as the writer's own. Ingest documents outside the server, stamping `org_id` exactly as the identity provider emits it. A tool meant to read across tenants, such as an internal support search, belongs on a separate server with its own authentication. Every tool on one server sits behind the same read scope, so any tenant could call it. @@ -415,6 +415,57 @@ Listing the tenant claim under `required_claims` makes the verifier reject a tok The server refuses to start an injecting profile without authentication, over `stdio`, or on a field the index does not hold as an indexed tag. The `stdio` check applies when the server starts through `rvl mcp` or `run_async`. For what the guarantee covers and what it rests on, read the threat model in {doc}`/concepts/mcp`. +### Code Tools Registered From Python + +When a tool needs logic a profile cannot express, write it as an async function and register it on the server before you run it. This suits a server you start from your own Python process: + +```python +from redisvl.mcp import MCPSettings, RedisVLMCPServer, ToolContext +from redisvl.query import FilterQuery +from redisvl.query.filter import Tag + +server = RedisVLMCPServer(MCPSettings(config="/path/to/mcp_config.yaml")) + + +@server.custom_tool(index="tickets") +async def open_tickets_for_product(ctx: ToolContext, product: str, limit: int = 5) -> dict: + """List the newest open support tickets for a product.""" + query = FilterQuery( + filter_expression=(Tag("status") == "open") & (Tag("product") == product), + return_fields=["subject", "created_at"], + num_results=limit, + ) + return {"tickets": await ctx.index.query(query)} + + +@server.custom_tool(index="tickets", writes=True, scope="tickets.triage") +async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: + """Mark a support ticket resolved.""" + ticket = await ctx.index.fetch(ticket_id) + if ticket is None: + raise ValueError(f"no ticket {ticket_id}") + await ctx.index.load([{**ticket, "status": "resolved"}], id_field="id") + return {"closed": ticket_id} + + +server.run(transport="streamable-http", host="127.0.0.1", port=8000) +``` + +What the client sees: + +- `open-tickets-for-product`, taking `product` (required) and `limit`, described by its docstring. +- `close-ticket`, taking `ticket_id`. A caller needs the server's write scope and `tickets.triage`. +- No argument for the context or the index, in either tool. + +Rules worth knowing: + +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. There is no Redis client and no way to create, drop or clear the index. +- **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. +- **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. +- **Register before starting.** Tools register once, at the first startup, so a tool registered afterwards is refused. + +A code tool bound to an index that a profile scopes with `lock.inject` refuses to start, because nothing shows that it applies the tenant scope. For the full contract, read the code tools section in {doc}`/concepts/mcp`. + ## Tool Contracts RedisVL MCP exposes a small, implementation-owned contract. @@ -803,8 +854,12 @@ A profile with `lock.inject` refuses to start when authentication is not enabled ### Claim Injection Refuses an Unscoped Route -A profile with `lock.inject` refuses to start while its Redis index is also reachable through `search-records`, through `upsert-records` on any writable binding over it, or through another custom tool on it whose `lock.inject` differs, including one with none. Bindings that share a `redis_name` are the same index. The error names the Redis index and the bindings over it, each route it found, and each tool's injected scope. Disable the built-ins under `server.builtin_tools`, mark every binding over the index `read_only`, and give every custom tool on the index the same `lock.inject`. Move a tool meant to read across tenants to a separate server with its own authentication. +A profile with `lock.inject` refuses to start while its Redis index is also reachable through `search-records`, through `upsert-records` on any writable binding over it, through another custom tool on it whose `lock.inject` differs, including one with none, or through a code tool bound to it. Bindings that share a `redis_name` are the same index. The error names the Redis index and the bindings over it, each route it found, and each tool's injected scope. Disable the built-ins under `server.builtin_tools`, mark every binding over the index `read_only`, and give every custom tool on the index the same `lock.inject`. Bind a code tool to another index. Move a tool meant to read across tenants to a separate server with its own authentication. ### Claim Injection Fails Every Request With `forbidden` The token is verified but its claim is unusable: missing, empty, padded with whitespace, not a single string, or containing a control character or backtick. The error names the claim and the tool. A misspelled `claim` name in the config is the usual cause, since a JWT claim name such as `https://acme.example/org` must match exactly. + +### Code Tool Fails at Startup + +The error names the tool. Either its `index` is missing while several indexes are configured or names no configured binding, or another custom tool already uses its name, or it is registered with `writes=True` on a binding or server that is read-only. A code tool registered after the server has started is refused with a `RuntimeError`; register every tool before calling `run`, `run_async` or `startup`. diff --git a/redisvl/mcp/__init__.py b/redisvl/mcp/__init__.py index 619c040c..5bcd8b0c 100644 --- a/redisvl/mcp/__init__.py +++ b/redisvl/mcp/__init__.py @@ -1,4 +1,5 @@ from redisvl.mcp.config import MCPConfig, MCPTransportSecurityConfig, load_mcp_config +from redisvl.mcp.context import ToolContext from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError, map_exception from redisvl.mcp.server import RedisVLMCPServer from redisvl.mcp.settings import MCPSettings @@ -10,6 +11,7 @@ "MCPTransportSecurityConfig", "RedisVLMCPError", "RedisVLMCPServer", + "ToolContext", "load_mcp_config", "map_exception", ] diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index ab550318..65daee08 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -1,10 +1,11 @@ import asyncio import logging +from collections.abc import Callable from contextlib import asynccontextmanager from enum import Enum, auto from importlib import import_module from pathlib import Path -from typing import Any, Awaitable +from typing import Any, Awaitable, TypeVar from redis import __version__ as redis_py_version @@ -15,6 +16,11 @@ from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.runtime import BindingRuntime from redisvl.mcp.settings import MCPSettings +from redisvl.mcp.tools.code import ( + CodeToolSpec, + build_code_tool_spec, + register_code_tool, +) from redisvl.mcp.tools.list_indexes import register_list_indexes_tool from redisvl.mcp.tools.profiles import ( register_profile_tools, @@ -32,6 +38,8 @@ logger = logging.getLogger(__name__) +_ToolFn = TypeVar("_ToolFn", bound=Callable[..., Any]) + def _describe_scope(scope: frozenset[tuple[str, str]]) -> str: """Render one tool's injected scope for a startup error.""" @@ -89,6 +97,10 @@ class RedisVLMCPServer(FastMCP): _LifecycleState = _LifecycleState + # Each code tool paired with the binding it resolved to, settled at startup + # once the config is loaded. Empty until then. + _resolved_code_tools: tuple[tuple[CodeToolSpec, str], ...] = () + def __init__(self, settings: MCPSettings): """Create a server shell with lazy config and per-binding runtime state.""" self.mcp_settings = settings @@ -98,6 +110,7 @@ def __init__(self, settings: MCPSettings): self._tools_registered = False self._registered_tool_fingerprint = "" self._registered_tools_inject = False + self._code_tools: list[CodeToolSpec] = [] # Set by run_async, the only point in the process that knows it. None # means an embedder started the server without naming a transport. self._transport: str | None = None @@ -213,6 +226,50 @@ async def shutdown(self) -> None: finally: await self._mark_stopped() + def custom_tool( + self, + *, + name: str | None = None, + namespace: str | None = None, + index: str | None = None, + writes: bool = False, + scope: str | None = None, + description: str | None = None, + ) -> Callable[[_ToolFn], _ToolFn]: + """Register an async function as a code tool on this server. + + The function takes a :class:`~redisvl.mcp.context.ToolContext` first and + the model's arguments after it; its docstring is the tool description + unless ``description`` is given. ``index`` pins the binding (required + when several are configured), ``writes`` grants the write methods and + requires the write scope, and ``scope`` is an extra scope a caller must + hold on top of the server-wide one. The function is returned unchanged. + + The function is checked here; whether its binding exists and permits it + is checked at startup, where the config is known. + """ + + def decorator(fn: _ToolFn) -> _ToolFn: + if self._tools_registered: + raise RuntimeError( + "MCP tools are registered once, at the first startup; " + "register code tools before starting the server" + ) + self._code_tools.append( + build_code_tool_spec( + fn, + name=name, + namespace=namespace, + index=index, + writes=writes, + scope=scope, + description=description, + ) + ) + return fn + + return decorator + def resolve_binding(self, index_id: str | None) -> BindingRuntime: """Resolve the runtime for a logical index id, honoring single-index defaults. @@ -389,7 +446,55 @@ def _verify_injection_has_a_token(self, profiles: Any) -> None: "streamable-http) so the configured auth applies." ) - def _verify_no_unscoped_route_to_injected_indexes(self, config: Any) -> None: + def _resolve_code_tools(self, config: Any) -> tuple[tuple[CodeToolSpec, str], ...]: + """Pair each code tool with its binding, refusing any that cannot hold. + + A code tool is declared in Python, so the rules the config model applies + to profiles are applied here instead, against the loaded config. + """ + taken = {profile.name for profile in config.custom_tools} + resolved: list[tuple[CodeToolSpec, str]] = [] + for spec in self._code_tools: + if spec.name in taken: + raise ValueError( + f"code tool name '{spec.name}' is already used by another " + "custom tool; tool names must be unique" + ) + taken.add(spec.name) + + if spec.index is None: + if len(config.indexes) > 1: + raise ValueError( + f"code tool '{spec.name}' must name its index because " + "several are configured: " + f"{', '.join(sorted(config.indexes))}" + ) + binding_id = next(iter(config.indexes)) + elif spec.index not in config.indexes: + raise ValueError( + f"code tool '{spec.name}' names unknown index '{spec.index}'; " + f"configured: {', '.join(sorted(config.indexes))}" + ) + else: + binding_id = spec.index + + # Refused here rather than per call, because the tool's writes were + # declared up front: it would fail on every call it exists for. + if spec.writes and ( + self.mcp_settings.read_only or config.indexes[binding_id].read_only + ): + raise ValueError( + f"code tool '{spec.name}' writes, but index '{binding_id}' is " + "read-only" + ) + resolved.append((spec, binding_id)) + return tuple(resolved) + + def _verify_no_unscoped_route_to_injected_indexes( + self, + config: Any, + code_tools: tuple[tuple[CodeToolSpec, str], ...] = (), + ) -> None: """Refuse a tool surface that reaches a tenant-scoped index unscoped. Injection isolates a tool, but tenant data lives in an index, and every @@ -406,6 +511,10 @@ def _verify_no_unscoped_route_to_injected_indexes(self, config: Any) -> None: tool scoped by another field, or by the same field from another claim, reads across the tenants the first one separates. + A code tool is a route too, and one the server cannot inspect: it can + run any query its handle allows, so nothing proves it applies the scope. + It is refused on a scoped index outright. + The index here is the Redis index, not the binding: two bindings with the same ``redis_name`` are two routes to one set of documents. The check sees only what this config names, so an alias, or a second index @@ -454,6 +563,19 @@ def _verify_no_unscoped_route_to_injected_indexes(self, config: Any) -> None: f"{', '.join(routes)}" ) + unscoped_code_tools = sorted( + spec.name + for spec, binding_id in code_tools + if config.indexes[binding_id].redis_name in injected + ) + if unscoped_code_tools: + problems.append( + "code tools " + f"{', '.join(repr(name) for name in unscoped_code_tools)} reach " + "it, and a code tool cannot carry an injected scope (bind it to " + "another index)" + ) + for redis_name in injected: by_tool = scopes[redis_name] if len(set(by_tool.values())) > 1: @@ -482,8 +604,7 @@ def _verify_no_unscoped_route_to_injected_indexes(self, config: Any) -> None: "on a separate server with its own authentication." ) - @staticmethod - def _tool_surface_fingerprint(config: Any) -> str: + def _tool_surface_fingerprint(self, config: Any) -> str: """Summarize the config that a registered tool set baked in.""" if config is None: return "" @@ -491,6 +612,12 @@ def _tool_surface_fingerprint(config: Any) -> str: ( sorted(config.server.builtin_tools.items()), [profile.model_dump(mode="json") for profile in config.custom_tools], + # The functions themselves cannot change between startups, but + # the binding each one resolved to depends on the config. + [ + (spec.name, binding_id) + for spec, binding_id in self._resolved_code_tools + ], ) ) @@ -581,6 +708,9 @@ def _register_tools(self) -> None: register_upsert_tool(self, index_ids=unlisted_index_ids) registered.append("upsert-records") registered.extend(register_profile_tools(self)) + for spec, binding_id in self._resolved_code_tools: + register_code_tool(self, spec, binding_id) + registered.append(spec.name) self._warn_on_unusable_tool_surface(registered) self._registered_tool_fingerprint = self._tool_surface_fingerprint(config) @@ -757,7 +887,10 @@ async def _initialize_runtime_resources(self) -> None: # Before any binding connects: both depend only on the loaded config, # so an unworkable one should not first need a reachable Redis. self._verify_injection_has_a_token(self.config.custom_tools) - self._verify_no_unscoped_route_to_injected_indexes(self.config) + self._resolved_code_tools = self._resolve_code_tools(self.config) + self._verify_no_unscoped_route_to_injected_indexes( + self.config, self._resolved_code_tools + ) # The semaphore is a single process-wide concurrency ceiling shared by # all bindings; take the max across bindings. This means the most # permissive binding sets the cap — e.g. five bindings each configured diff --git a/tests/integration/test_mcp/test_code_tools.py b/tests/integration/test_mcp/test_code_tools.py new file mode 100644 index 00000000..2a403f20 --- /dev/null +++ b/tests/integration/test_mcp/test_code_tools.py @@ -0,0 +1,224 @@ +"""Code tools registered with ``server.custom_tool``, against a real Redis. + +The unit suites own the validation and wrapping matrices. What lives here is +what depends on a started server: that a tool's handle reaches the real index, +that a writing tool really writes, that startup refuses what the config cannot +hold, and that the concurrency cap governs each Redis call without deadlocking +a tool that makes several. +""" + +import asyncio +from pathlib import Path + +import pytest +import yaml + +from redisvl.index import AsyncSearchIndex +from redisvl.mcp import ToolContext +from redisvl.mcp.server import RedisVLMCPServer +from redisvl.mcp.settings import MCPSettings +from redisvl.query import FilterQuery +from redisvl.query.filter import Tag +from redisvl.schema import IndexSchema + +fastmcp = pytest.importorskip( + "fastmcp", reason="fastmcp not installed (install redisvl[mcp])" +) +from fastmcp.server.auth.providers.jwt import RSAKeyPair + + +@pytest.fixture +async def tickets_index(async_client, worker_id): + schema = IndexSchema.from_dict( + { + "index": { + "name": f"mcp-code-tools-{worker_id}", + "prefix": f"mcp-code-tools:{worker_id}", + "storage_type": "hash", + }, + "fields": [ + {"name": "content", "type": "text"}, + {"name": "status", "type": "tag"}, + ], + } + ) + index = AsyncSearchIndex(schema=schema, redis_client=async_client) + await index.create(overwrite=True, drop=True) + await index.load( + [ + {"id": "t1", "content": "refund delayed", "status": "open"}, + {"id": "t2", "content": "refund issued", "status": "resolved"}, + {"id": "t3", "content": "login failure", "status": "open"}, + ], + id_field="id", + ) + yield index + await index.delete(drop=True) + + +@pytest.fixture +def config_path(tmp_path: Path, redis_url: str): + def factory( + redis_name: str, + *, + read_only: bool = False, + max_concurrency: int = 16, + custom_tools: list[dict] | None = None, + auth: dict | None = None, + ) -> str: + server: dict = {"redis_url": redis_url} + if auth is not None: + server["auth"] = auth + server["builtin_tools"] = { + "search-records": "disabled", + "upsert-records": "disabled", + } + config = { + "server": server, + "indexes": { + "tickets": { + "redis_name": redis_name, + "read_only": read_only, + "search": {"type": "fulltext"}, + "runtime": { + "text_field_name": "content", + "max_concurrency": max_concurrency, + }, + } + }, + "custom_tools": custom_tools or [], + } + path = tmp_path / f"{redis_name}-code-tools.yaml" + path.write_text(yaml.safe_dump(config), encoding="utf-8") + return str(path) + + return factory + + +@pytest.fixture +async def server_for(tickets_index, config_path): + servers: list[RedisVLMCPServer] = [] + + def build(**kwargs) -> RedisVLMCPServer: + server = RedisVLMCPServer( + MCPSettings(config=config_path(tickets_index.schema.index.name, **kwargs)) + ) + servers.append(server) + return server + + yield build + + for server in servers: + await server.shutdown() + + +async def _call(server: RedisVLMCPServer, name: str, **kwargs): + tool = await server.get_tool(name) + assert tool is not None, f"tool '{name}' is not registered" + return await tool.fn(**kwargs) + + +async def test_a_read_tool_queries_the_bound_index(server_for): + server = server_for() + + @server.custom_tool() + async def open_tickets(ctx: ToolContext, limit: int = 10) -> dict: + """List open support tickets.""" + query = FilterQuery( + filter_expression=Tag("status") == "open", + return_fields=["content"], + num_results=limit, + ) + return {"tickets": sorted(r["content"] for r in await ctx.index.query(query))} + + await server.startup() + + assert await _call(server, "open-tickets") == { + "tickets": ["login failure", "refund delayed"] + } + + +async def test_a_writing_tool_writes_through_its_handle(server_for): + server = server_for() + + @server.custom_tool(writes=True) + async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: + """Mark a support ticket resolved.""" + record = await ctx.index.fetch(ticket_id) + await ctx.index.load([{**record, "status": "resolved"}], id_field="id") + return {"status": (await ctx.index.fetch(ticket_id))["status"]} + + await server.startup() + + assert await _call(server, "close-ticket", ticket_id="t1") == {"status": "resolved"} + + +async def test_a_writing_tool_on_a_read_only_index_fails_startup(server_for): + server = server_for(read_only=True) + + @server.custom_tool(writes=True) + async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: + """Mark a support ticket resolved.""" + return {} + + with pytest.raises(ValueError, match="'close-ticket' writes, but index"): + await server.startup() + + +async def test_a_code_tool_on_a_tenant_scoped_index_fails_startup(server_for): + key = RSAKeyPair.generate() + server = server_for( + auth={ + "type": "jwt", + "public_key": key.public_key, + "issuer": "https://auth.acme.example/", + "audience": "api://redisvl-mcp", + }, + custom_tools=[ + { + "name": "tenant-tickets", + "description": "Search this tenant's tickets.", + "lock": { + "inject": [{"field": "status", "from": "claim", "claim": "org"}] + }, + } + ], + ) + # Startup only checks for a token source on HTTP; naming one keeps the + # refusal under test the code-tool one. + server._transport = "streamable-http" + + @server.custom_tool() + async def all_tickets(ctx: ToolContext) -> dict: + """List every ticket.""" + return {} + + with pytest.raises(ValueError, match="code tools 'all-tickets' reach it"): + await server.startup() + + +async def test_each_redis_call_takes_the_cap_without_deadlocking_the_tool( + server_for, +): + # With a cap of one, a tool holding the cap across its whole body would + # block its own second call. The cap is per Redis call, so two concurrent + # calls of a tool that queries twice each still finish. + server = server_for(max_concurrency=1) + + @server.custom_tool() + async def ticket_counts(ctx: ToolContext) -> dict: + """Count open and resolved tickets.""" + counts = {} + for status in ("open", "resolved"): + query = FilterQuery(filter_expression=Tag("status") == status) + counts[status] = len(await ctx.index.query(query)) + return counts + + await server.startup() + + results = await asyncio.wait_for( + asyncio.gather(_call(server, "ticket-counts"), _call(server, "ticket-counts")), + timeout=10, + ) + + assert results == [{"open": 2, "resolved": 1}] * 2 diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 70c72ec0..1b0e8c71 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -4,6 +4,7 @@ import pytest from redisvl.mcp.config import MCPConfig, builtin_tool_names +from redisvl.mcp.context import ToolContext from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.runtime import BindingRuntime from redisvl.mcp.server import RedisVLMCPServer @@ -1042,3 +1043,243 @@ def test_tools_injecting_the_same_scope_share_an_index(): }, ], ) + + +# -------------------------------------------------------------------------- +# Code tools registered with server.custom_tool +# -------------------------------------------------------------------------- + + +async def recent_tickets(ctx: ToolContext, query: str) -> dict: + """Find recent tickets.""" + return {} + + +def _code_tool_server(*, read_only=False) -> RedisVLMCPServer: + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._code_tools = [] + server._tools_registered = False + server.mcp_settings = SimpleNamespace(read_only=read_only) + return server + + +def _two_index_config(*, custom_tools=None, **knowledge_overrides) -> MCPConfig: + return MCPConfig.model_validate( + { + "server": {"redis_url": "redis://localhost:6379"}, + "indexes": { + "knowledge": { + "redis_name": "docs-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + **knowledge_overrides, + }, + "tickets": { + "redis_name": "tickets-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + }, + }, + "custom_tools": [ + {**tool, "index": tool.get("index", "knowledge")} + for tool in custom_tools or [] + ], + } + ) + + +def test_custom_tool_records_the_tool_and_returns_the_function(): + server = _code_tool_server() + + decorated = server.custom_tool(namespace="support")(recent_tickets) + + assert decorated is recent_tickets + assert [spec.name for spec in server._code_tools] == ["support-recent-tickets"] + + +def test_custom_tool_checks_the_function_where_it_is_declared(): + server = _code_tool_server() + + def sync_tool(ctx: ToolContext, query: str) -> dict: + """Search.""" + return {} + + with pytest.raises(TypeError, match="must be an async function"): + server.custom_tool()(sync_tool) + assert server._code_tools == [] + + +def test_custom_tool_after_registration_is_refused(): + server = _code_tool_server() + server._tools_registered = True + + with pytest.raises(RuntimeError, match="register code tools before starting"): + server.custom_tool()(recent_tickets) + + +def test_a_code_tool_on_a_single_index_server_binds_to_it(): + server = _code_tool_server() + server.custom_tool()(recent_tickets) + + ((spec, binding_id),) = server._resolve_code_tools(_config_with()) + + assert (spec.name, binding_id) == ("recent-tickets", "knowledge") + + +def test_a_code_tool_binds_to_the_index_it_names(): + server = _code_tool_server() + server.custom_tool(index="tickets")(recent_tickets) + + ((_, binding_id),) = server._resolve_code_tools(_two_index_config()) + + assert binding_id == "tickets" + + +@pytest.mark.parametrize( + "options, config, expected", + [ + pytest.param( + {}, + _two_index_config(), + "must name its index because several are configured: knowledge, tickets", + id="index-required", + ), + pytest.param( + {"index": "archive"}, + _two_index_config(), + "unknown index 'archive'; configured: knowledge, tickets", + id="unknown-index", + ), + pytest.param( + {"index": "knowledge", "name": "resolved-search"}, + _two_index_config( + custom_tools=[{"name": "resolved-search", "description": "Search."}] + ), + "'resolved-search' is already used by another custom tool", + id="profile-name", + ), + pytest.param( + {"index": "knowledge", "writes": True}, + _two_index_config(read_only=True), + "'recent-tickets' writes, but index 'knowledge' is read-only", + id="binding-read-only", + ), + ], +) +def test_a_code_tool_its_config_cannot_hold_fails_startup(options, config, expected): + server = _code_tool_server() + server.custom_tool(**options)(recent_tickets) + + with pytest.raises(ValueError, match=expected): + server._resolve_code_tools(config) + + +def test_two_code_tools_cannot_share_a_name(): + server = _code_tool_server() + server.custom_tool()(recent_tickets) + server.custom_tool()(recent_tickets) + + with pytest.raises(ValueError, match="'recent-tickets' is already used"): + server._resolve_code_tools(_config_with()) + + +def test_a_writing_code_tool_is_refused_on_a_read_only_server(): + server = _code_tool_server(read_only=True) + server.custom_tool(writes=True)(recent_tickets) + + with pytest.raises(ValueError, match="is read-only"): + server._resolve_code_tools(_config_with()) + + +def test_a_reading_code_tool_may_use_a_read_only_index(): + server = _code_tool_server(read_only=True) + server.custom_tool()(recent_tickets) + + assert len(server._resolve_code_tools(_config_with())) == 1 + + +def _code_tool_route_check(*, index, indexes): + server = _code_tool_server() + server.custom_tool(index=index)(recent_tickets) + config = MCPConfig.model_validate( + { + "server": { + "redis_url": "redis://localhost:6379", + "builtin_tools": _BUILTINS_OFF, + }, + "indexes": { + index_id: { + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + **overrides, + } + for index_id, overrides in indexes.items() + }, + "custom_tools": [{**_INJECTING, "index": "kb_semantic"}], + } + ) + server._verify_no_unscoped_route_to_injected_indexes( + config, server._resolve_code_tools(config) + ) + + +@pytest.mark.parametrize( + "index", + [ + pytest.param("kb_semantic", id="same-binding"), + # A second binding over the same Redis index reaches the same documents. + pytest.param("kb_keyword", id="sibling-binding"), + ], +) +def test_a_code_tool_on_a_tenant_scoped_index_is_refused(index): + with pytest.raises(ValueError, match="code tools 'recent-tickets' reach it"): + _code_tool_route_check( + index=index, + indexes={ + "kb_semantic": {"redis_name": "customer-kb"}, + "kb_keyword": {"redis_name": "customer-kb"}, + }, + ) + + +def test_a_code_tool_on_another_index_is_no_route(): + _code_tool_route_check( + index="public", + indexes={ + "kb_semantic": {"redis_name": "customer-kb"}, + "public": {"redis_name": "public-kb"}, + }, + ) + + +def test_register_tools_registers_code_tools_after_profiles(monkeypatch): + calls: list[tuple[str, str]] = [] + monkeypatch.setattr( + "redisvl.mcp.server.register_code_tool", + lambda server, spec, binding_id: calls.append((spec.name, binding_id)), + ) + server = _code_tool_server() + server.custom_tool()(recent_tickets) + config = _config_with( + custom_tools=[{"name": "resolved-search", "description": "Search."}] + ) + resolved = server._resolve_code_tools(config) + monkeypatch.setattr(RedisVLMCPServer, "_resolved_code_tools", resolved) + + registered = _register_tools_with( + monkeypatch, {"knowledge": _binding_runtime("knowledge")}, config=config + ) + + assert registered[-1] == "resolved-search" + assert calls == [("recent-tickets", "knowledge")] + + +def test_the_tool_surface_fingerprint_covers_code_tools(): + server = _code_tool_server() + config = _config_with() + without = server._tool_surface_fingerprint(config) + + server.custom_tool()(recent_tickets) + server._resolved_code_tools = server._resolve_code_tools(config) + + assert server._tool_surface_fingerprint(config) != without From 8c3a96b4768717ed63528e584d616332e6034d9c Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 14 Oct 2026 10:21:55 +0200 Subject: [PATCH 08/15] fix(mcp): name the API code_tool and refuse a restart that changes code tools server.custom_tool becomes server.code_tool, matching allow_code_tools and the docs, and no longer reading as a way to register a profile, which custom_tools already means. A restart that would pair the registered code tools with different bindings now fails instead of warning, since the old pairing would stay live. A writing tool on a read-only server says so rather than blaming the index. The docs describe the id-only writes and the SQLQuery refusal, replace an example that assumed the record carried its own id, and name code tools in the tool surface. --- docs/concepts/mcp.md | 10 ++- docs/user_guide/how_to_guides/mcp.md | 21 +++-- redisvl/mcp/server.py | 42 ++++++---- redisvl/mcp/tools/code.py | 4 +- tests/integration/test_mcp/test_code_tools.py | 22 ++--- tests/unit/test_mcp/test_server_unit.py | 81 +++++++++++-------- 6 files changed, 98 insertions(+), 82 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 7043feff..666a2a66 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -105,7 +105,7 @@ For configuration and the gateway boundary, see {doc}`/user_guide/how_to_guides/ ## Tool Surface -RedisVL MCP exposes up to three built-in tools, plus any configured [custom tool profiles](#custom-tool-profiles): +RedisVL MCP exposes up to three built-in tools, plus any configured [custom tool profiles](#custom-tool-profiles) and [code tools](#code-tools): - `list-indexes` enumerates the configured logical indexes for discovery - `search-records` searches a selected index using that index's server-owned search mode @@ -276,10 +276,10 @@ from redisvl.mcp import MCPSettings, RedisVLMCPServer, ToolContext from redisvl.query import CountQuery from redisvl.query.filter import Tag -server = RedisVLMCPServer(MCPSettings(config="mcp.yaml")) +server = RedisVLMCPServer(MCPSettings(config="/path/to/mcp_config.yaml")) -@server.custom_tool(index="tickets") +@server.code_tool(index="tickets") async def count_open_tickets(ctx: ToolContext, product: str) -> dict: """Count open support tickets for a product.""" open_for_product = (Tag("status") == "open") & (Tag("product") == product) @@ -296,11 +296,13 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | Attribute | What it is | |---|---| -| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_keys`, `drop_documents` and `expire_keys`. | +| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. | | `ctx.schema` | The bound index's schema. | | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | | `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the request has no token. | +Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart. + Everything else on the index is out of reach: creating, dropping or clearing it, `info` for another index, `paginate`, and the filter-wide bulk writes `update_by_filter` and `drop_by_filter`, which a request timeout could cancel partway through. The server applies the same rails as for a built-in, without the author asking for them: diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 7487e11f..6f35c0a6 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -427,9 +427,9 @@ from redisvl.query.filter import Tag server = RedisVLMCPServer(MCPSettings(config="/path/to/mcp_config.yaml")) -@server.custom_tool(index="tickets") +@server.code_tool(index="tickets") async def open_tickets_for_product(ctx: ToolContext, product: str, limit: int = 5) -> dict: - """List the newest open support tickets for a product.""" + """List open support tickets for a product.""" query = FilterQuery( filter_expression=(Tag("status") == "open") & (Tag("product") == product), return_fields=["subject", "created_at"], @@ -438,14 +438,13 @@ async def open_tickets_for_product(ctx: ToolContext, product: str, limit: int = return {"tickets": await ctx.index.query(query)} -@server.custom_tool(index="tickets", writes=True, scope="tickets.triage") -async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: - """Mark a support ticket resolved.""" +@server.code_tool(index="tickets", writes=True, scope="tickets.triage") +async def delete_spam_ticket(ctx: ToolContext, ticket_id: str) -> dict: + """Delete a support ticket that triage has marked as spam.""" ticket = await ctx.index.fetch(ticket_id) - if ticket is None: - raise ValueError(f"no ticket {ticket_id}") - await ctx.index.load([{**ticket, "status": "resolved"}], id_field="id") - return {"closed": ticket_id} + if ticket is None or ticket.get("status") != "spam": + raise ValueError(f"ticket {ticket_id} is not marked as spam") + return {"deleted": await ctx.index.drop_documents(ticket_id)} server.run(transport="streamable-http", host="127.0.0.1", port=8000) @@ -454,12 +453,12 @@ server.run(transport="streamable-http", host="127.0.0.1", port=8000) What the client sees: - `open-tickets-for-product`, taking `product` (required) and `limit`, described by its docstring. -- `close-ticket`, taking `ticket_id`. A caller needs the server's write scope and `tickets.triage`. +- `delete-spam-ticket`, taking `ticket_id`. A caller needs the server's write scope and `tickets.triage`, and the tool deletes nothing that is not already marked as spam. - No argument for the context or the index, in either tool. Rules worth knowing: -- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. There is no Redis client and no way to create, drop or clear the index. +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool cannot touch another index's keys. There is no Redis client and no way to create, drop or clear the index. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. - **Register before starting.** Tools register once, at the first startup, so a tool registered afterwards is refused. diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 65daee08..e97f28ef 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -5,7 +5,7 @@ from enum import Enum, auto from importlib import import_module from pathlib import Path -from typing import Any, Awaitable, TypeVar +from typing import Any, Awaitable from redis import __version__ as redis_py_version @@ -18,6 +18,7 @@ from redisvl.mcp.settings import MCPSettings from redisvl.mcp.tools.code import ( CodeToolSpec, + _ToolFn, build_code_tool_spec, register_code_tool, ) @@ -38,8 +39,6 @@ logger = logging.getLogger(__name__) -_ToolFn = TypeVar("_ToolFn", bound=Callable[..., Any]) - def _describe_scope(scope: frozenset[tuple[str, str]]) -> str: """Render one tool's injected scope for a startup error.""" @@ -97,9 +96,11 @@ class RedisVLMCPServer(FastMCP): _LifecycleState = _LifecycleState - # Each code tool paired with the binding it resolved to, settled at startup - # once the config is loaded. Empty until then. + # Each code tool paired with the binding it resolved to, settled at every + # startup once the config is loaded, and the pairing the tools were + # registered with at the first. Empty until then. _resolved_code_tools: tuple[tuple[CodeToolSpec, str], ...] = () + _registered_code_tools: tuple[tuple[CodeToolSpec, str], ...] = () def __init__(self, settings: MCPSettings): """Create a server shell with lazy config and per-binding runtime state.""" @@ -226,7 +227,7 @@ async def shutdown(self) -> None: finally: await self._mark_stopped() - def custom_tool( + def code_tool( self, *, name: str | None = None, @@ -480,9 +481,11 @@ def _resolve_code_tools(self, config: Any) -> tuple[tuple[CodeToolSpec, str], .. # Refused here rather than per call, because the tool's writes were # declared up front: it would fail on every call it exists for. - if spec.writes and ( - self.mcp_settings.read_only or config.indexes[binding_id].read_only - ): + if spec.writes and self.mcp_settings.read_only: + raise ValueError( + f"code tool '{spec.name}' writes, but the server runs read-only" + ) + if spec.writes and config.indexes[binding_id].read_only: raise ValueError( f"code tool '{spec.name}' writes, but index '{binding_id}' is " "read-only" @@ -604,7 +607,8 @@ def _verify_no_unscoped_route_to_injected_indexes( "on a separate server with its own authentication." ) - def _tool_surface_fingerprint(self, config: Any) -> str: + @staticmethod + def _tool_surface_fingerprint(config: Any) -> str: """Summarize the config that a registered tool set baked in.""" if config is None: return "" @@ -612,12 +616,6 @@ def _tool_surface_fingerprint(self, config: Any) -> str: ( sorted(config.server.builtin_tools.items()), [profile.model_dump(mode="json") for profile in config.custom_tools], - # The functions themselves cannot change between startups, but - # the binding each one resolved to depends on the config. - [ - (spec.name, binding_id) - for spec, binding_id in self._resolved_code_tools - ], ) ) @@ -634,6 +632,17 @@ def _register_tools(self) -> None: # The dangerous direction is an operator disabling a tool or tightening # a lock and believing the restart applied it. if self._tools_registered: + # A code tool runs operator code against whatever binding it + # was registered on, so a restart that would pair it with a + # different one, or drop it, is never just a log line. + if self._resolved_code_tools != self._registered_code_tools: + raise RuntimeError( + "MCP code tools, or the indexes they are bound to, " + "changed since tools were registered. Tools register " + "once per process, so the previously registered code " + "tools would stay in effect. Restart the process to " + "apply the new configuration." + ) config = getattr(self, "config", None) current = self._tool_surface_fingerprint(config) if current != self._registered_tool_fingerprint: @@ -714,6 +723,7 @@ def _register_tools(self) -> None: self._warn_on_unusable_tool_surface(registered) self._registered_tool_fingerprint = self._tool_surface_fingerprint(config) + self._registered_code_tools = self._resolved_code_tools self._registered_tools_inject = _config_injects(config) self._tools_registered = True diff --git a/redisvl/mcp/tools/code.py b/redisvl/mcp/tools/code.py index 2c3bba83..38ea48d8 100644 --- a/redisvl/mcp/tools/code.py +++ b/redisvl/mcp/tools/code.py @@ -18,7 +18,7 @@ import typing from collections.abc import Callable from dataclasses import dataclass -from typing import Any +from typing import Any, TypeVar from pydantic import TypeAdapter @@ -27,6 +27,8 @@ from redisvl.mcp.context import ToolContext from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError, map_exception +_ToolFn = TypeVar("_ToolFn", bound=Callable[..., Any]) + _ACCEPTED_KINDS = ( inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY, diff --git a/tests/integration/test_mcp/test_code_tools.py b/tests/integration/test_mcp/test_code_tools.py index 2a403f20..cacdf38d 100644 --- a/tests/integration/test_mcp/test_code_tools.py +++ b/tests/integration/test_mcp/test_code_tools.py @@ -1,4 +1,4 @@ -"""Code tools registered with ``server.custom_tool``, against a real Redis. +"""Code tools registered with ``server.code_tool``, against a real Redis. The unit suites own the validation and wrapping matrices. What lives here is what depends on a started server: that a tool's handle reaches the real index, @@ -121,7 +121,7 @@ async def _call(server: RedisVLMCPServer, name: str, **kwargs): async def test_a_read_tool_queries_the_bound_index(server_for): server = server_for() - @server.custom_tool() + @server.code_tool() async def open_tickets(ctx: ToolContext, limit: int = 10) -> dict: """List open support tickets.""" query = FilterQuery( @@ -141,7 +141,7 @@ async def open_tickets(ctx: ToolContext, limit: int = 10) -> dict: async def test_a_writing_tool_writes_through_its_handle(server_for): server = server_for() - @server.custom_tool(writes=True) + @server.code_tool(writes=True) async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: """Mark a support ticket resolved.""" record = await ctx.index.fetch(ticket_id) @@ -153,18 +153,6 @@ async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: assert await _call(server, "close-ticket", ticket_id="t1") == {"status": "resolved"} -async def test_a_writing_tool_on_a_read_only_index_fails_startup(server_for): - server = server_for(read_only=True) - - @server.custom_tool(writes=True) - async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: - """Mark a support ticket resolved.""" - return {} - - with pytest.raises(ValueError, match="'close-ticket' writes, but index"): - await server.startup() - - async def test_a_code_tool_on_a_tenant_scoped_index_fails_startup(server_for): key = RSAKeyPair.generate() server = server_for( @@ -188,7 +176,7 @@ async def test_a_code_tool_on_a_tenant_scoped_index_fails_startup(server_for): # refusal under test the code-tool one. server._transport = "streamable-http" - @server.custom_tool() + @server.code_tool() async def all_tickets(ctx: ToolContext) -> dict: """List every ticket.""" return {} @@ -205,7 +193,7 @@ async def test_each_redis_call_takes_the_cap_without_deadlocking_the_tool( # calls of a tool that queries twice each still finish. server = server_for(max_concurrency=1) - @server.custom_tool() + @server.code_tool() async def ticket_counts(ctx: ToolContext) -> dict: """Count open and resolved tickets.""" counts = {} diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 1b0e8c71..cbde2416 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -8,6 +8,7 @@ from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.runtime import BindingRuntime from redisvl.mcp.server import RedisVLMCPServer +from redisvl.mcp.tools.code import build_code_tool_spec class FakeClient: @@ -147,9 +148,15 @@ async def fake_close_resources(self, *, index, vectorizer): assert server._tools_registered is True -def _register_tools_with(monkeypatch, bindings: dict, *, config=None) -> list[str]: +def _register_tools_with( + monkeypatch, bindings: dict, *, config=None, code_tools=() +) -> list[str]: """Run _register_tools against the given bindings, returning registered names.""" registered: list[str] = [] + monkeypatch.setattr( + "redisvl.mcp.server.register_code_tool", + lambda server, spec, binding_id: registered.append(spec.name), + ) monkeypatch.setattr( "redisvl.mcp.server.register_list_indexes_tool", lambda server: registered.append("list-indexes"), @@ -186,6 +193,7 @@ def fake_register_profile_tools(server): server.tool = object() server.config = config server.mcp_settings = SimpleNamespace(read_only=False) + server._resolved_code_tools = tuple(code_tools) server._register_tools() return registered @@ -1046,7 +1054,7 @@ def test_tools_injecting_the_same_scope_share_an_index(): # -------------------------------------------------------------------------- -# Code tools registered with server.custom_tool +# Code tools registered with server.code_tool # -------------------------------------------------------------------------- @@ -1091,7 +1099,7 @@ def _two_index_config(*, custom_tools=None, **knowledge_overrides) -> MCPConfig: def test_custom_tool_records_the_tool_and_returns_the_function(): server = _code_tool_server() - decorated = server.custom_tool(namespace="support")(recent_tickets) + decorated = server.code_tool(namespace="support")(recent_tickets) assert decorated is recent_tickets assert [spec.name for spec in server._code_tools] == ["support-recent-tickets"] @@ -1105,7 +1113,7 @@ def sync_tool(ctx: ToolContext, query: str) -> dict: return {} with pytest.raises(TypeError, match="must be an async function"): - server.custom_tool()(sync_tool) + server.code_tool()(sync_tool) assert server._code_tools == [] @@ -1114,12 +1122,12 @@ def test_custom_tool_after_registration_is_refused(): server._tools_registered = True with pytest.raises(RuntimeError, match="register code tools before starting"): - server.custom_tool()(recent_tickets) + server.code_tool()(recent_tickets) def test_a_code_tool_on_a_single_index_server_binds_to_it(): server = _code_tool_server() - server.custom_tool()(recent_tickets) + server.code_tool()(recent_tickets) ((spec, binding_id),) = server._resolve_code_tools(_config_with()) @@ -1128,7 +1136,7 @@ def test_a_code_tool_on_a_single_index_server_binds_to_it(): def test_a_code_tool_binds_to_the_index_it_names(): server = _code_tool_server() - server.custom_tool(index="tickets")(recent_tickets) + server.code_tool(index="tickets")(recent_tickets) ((_, binding_id),) = server._resolve_code_tools(_two_index_config()) @@ -1168,7 +1176,7 @@ def test_a_code_tool_binds_to_the_index_it_names(): ) def test_a_code_tool_its_config_cannot_hold_fails_startup(options, config, expected): server = _code_tool_server() - server.custom_tool(**options)(recent_tickets) + server.code_tool(**options)(recent_tickets) with pytest.raises(ValueError, match=expected): server._resolve_code_tools(config) @@ -1176,8 +1184,8 @@ def test_a_code_tool_its_config_cannot_hold_fails_startup(options, config, expec def test_two_code_tools_cannot_share_a_name(): server = _code_tool_server() - server.custom_tool()(recent_tickets) - server.custom_tool()(recent_tickets) + server.code_tool()(recent_tickets) + server.code_tool()(recent_tickets) with pytest.raises(ValueError, match="'recent-tickets' is already used"): server._resolve_code_tools(_config_with()) @@ -1185,22 +1193,22 @@ def test_two_code_tools_cannot_share_a_name(): def test_a_writing_code_tool_is_refused_on_a_read_only_server(): server = _code_tool_server(read_only=True) - server.custom_tool(writes=True)(recent_tickets) + server.code_tool(writes=True)(recent_tickets) - with pytest.raises(ValueError, match="is read-only"): + with pytest.raises(ValueError, match="writes, but the server runs read-only"): server._resolve_code_tools(_config_with()) -def test_a_reading_code_tool_may_use_a_read_only_index(): +def test_a_reading_code_tool_may_run_on_a_read_only_server(): server = _code_tool_server(read_only=True) - server.custom_tool()(recent_tickets) + server.code_tool()(recent_tickets) assert len(server._resolve_code_tools(_config_with())) == 1 def _code_tool_route_check(*, index, indexes): server = _code_tool_server() - server.custom_tool(index=index)(recent_tickets) + server.code_tool(index=index)(recent_tickets) config = MCPConfig.model_validate( { "server": { @@ -1253,33 +1261,40 @@ def test_a_code_tool_on_another_index_is_no_route(): def test_register_tools_registers_code_tools_after_profiles(monkeypatch): - calls: list[tuple[str, str]] = [] - monkeypatch.setattr( - "redisvl.mcp.server.register_code_tool", - lambda server, spec, binding_id: calls.append((spec.name, binding_id)), - ) server = _code_tool_server() - server.custom_tool()(recent_tickets) + server.code_tool()(recent_tickets) config = _config_with( custom_tools=[{"name": "resolved-search", "description": "Search."}] ) - resolved = server._resolve_code_tools(config) - monkeypatch.setattr(RedisVLMCPServer, "_resolved_code_tools", resolved) registered = _register_tools_with( - monkeypatch, {"knowledge": _binding_runtime("knowledge")}, config=config + monkeypatch, + {"knowledge": _binding_runtime("knowledge")}, + config=config, + code_tools=server._resolve_code_tools(config), ) - assert registered[-1] == "resolved-search" - assert calls == [("recent-tickets", "knowledge")] + assert registered[-2:] == ["resolved-search", "recent-tickets"] -def test_the_tool_surface_fingerprint_covers_code_tools(): - server = _code_tool_server() - config = _config_with() - without = server._tool_surface_fingerprint(config) +def _restart_with(*, registered_on, restarted_on): + """Register a code tool on one binding, then restart pairing it with another.""" + spec = build_code_tool_spec(recent_tickets) + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._tools_registered = True + server._registered_code_tools = ((spec, registered_on),) + server._resolved_code_tools = ((spec, restarted_on),) + server._registered_tool_fingerprint = server._tool_surface_fingerprint(None) + server._registered_tools_inject = False + server.tool = object() + server.config = None + server._register_tools() + + +def test_a_restart_that_moves_a_code_tool_is_fatal(): + with pytest.raises(RuntimeError, match="code tools, or the indexes they are"): + _restart_with(registered_on="knowledge", restarted_on="tickets") - server.custom_tool()(recent_tickets) - server._resolved_code_tools = server._resolve_code_tools(config) - assert server._tool_surface_fingerprint(config) != without +def test_a_restart_with_the_same_code_tools_proceeds(): + _restart_with(registered_on="knowledge", restarted_on="knowledge") From faacb231d8ce6b5f1992892fd6f9e61977db209a Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 20 Oct 2026 11:49:22 +0200 Subject: [PATCH 09/15] docs(mcp): say scopes need auth, qualify key isolation, and export the handle types --- docs/concepts/mcp.md | 4 ++-- docs/user_guide/how_to_guides/mcp.md | 5 +++-- redisvl/mcp/__init__.py | 4 +++- redisvl/mcp/auth.py | 1 - redisvl/mcp/server.py | 4 +++- tests/integration/test_mcp/test_code_tools.py | 20 ------------------- 6 files changed, 11 insertions(+), 27 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 666a2a66..54dd1cc3 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -301,7 +301,7 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | | `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the request has no token. | -Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart. +Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart, and an index with an empty prefix covers every key, so its document ids are full Redis keys. Everything else on the index is out of reach: creating, dropping or clearing it, `info` for another index, `paginate`, and the filter-wide bulk writes `update_by_filter` and `drop_by_filter`, which a request timeout could cancel partway through. @@ -309,7 +309,7 @@ The server applies the same rails as for a built-in, without the author asking f - Every `ctx.index` call takes a slot under the server-wide concurrency cap and is bounded by the binding's `runtime.request_timeout_seconds`. The cap is taken per call, so a tool that queries several times never blocks itself. - The whole tool call is bounded by the same timeout, so a tool cannot hold a request open indefinitely. A timeout returns a retryable `backend_unavailable` error. -- A caller needs the server's read scope, or the write scope for a tool declared with `writes=True`. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. +- A caller needs the server's read scope, or the write scope for a tool declared with `writes=True`. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. Like every scope, it is enforced only when the server has authentication configured. - An exception the tool raises becomes a stable MCP error: `ValueError` becomes `invalid_request`, a Redis error a retryable `backend_unavailable`, most other exceptions `internal_error`. Raise `RedisVLMCPError` to choose the code and message yourself. The handle is a guardrail, not a sandbox. A code tool is your own code, running with the privileges of the process that started the server, and Python can still reach private attributes. What the handle does is make the bounded, policy-checked path the only obvious one. diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 6f35c0a6..265c130a 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -453,12 +453,13 @@ server.run(transport="streamable-http", host="127.0.0.1", port=8000) What the client sees: - `open-tickets-for-product`, taking `product` (required) and `limit`, described by its docstring. -- `delete-spam-ticket`, taking `ticket_id`. A caller needs the server's write scope and `tickets.triage`, and the tool deletes nothing that is not already marked as spam. +- `delete-spam-ticket`, taking `ticket_id`. With authentication configured, a caller needs the server's write scope and `tickets.triage`; without it, no scope is checked. Either way, the tool deletes nothing that is not already marked as spam. - No argument for the context or the index, in either tool. Rules worth knowing: -- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool cannot touch another index's keys. There is no Redis client and no way to create, drop or clear the index. +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. +- **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods. Both types are exported from `redisvl.mcp`. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. - **Register before starting.** Tools register once, at the first startup, so a tool registered afterwards is refused. diff --git a/redisvl/mcp/__init__.py b/redisvl/mcp/__init__.py index 5bcd8b0c..a94bae1b 100644 --- a/redisvl/mcp/__init__.py +++ b/redisvl/mcp/__init__.py @@ -1,10 +1,11 @@ from redisvl.mcp.config import MCPConfig, MCPTransportSecurityConfig, load_mcp_config -from redisvl.mcp.context import ToolContext +from redisvl.mcp.context import IndexHandle, ToolContext, WritableIndexHandle from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError, map_exception from redisvl.mcp.server import RedisVLMCPServer from redisvl.mcp.settings import MCPSettings __all__ = [ + "IndexHandle", "MCPConfig", "MCPErrorCode", "MCPSettings", @@ -12,6 +13,7 @@ "RedisVLMCPError", "RedisVLMCPServer", "ToolContext", + "WritableIndexHandle", "load_mcp_config", "map_exception", ] diff --git a/redisvl/mcp/auth.py b/redisvl/mcp/auth.py index d2c8e462..666e8913 100644 --- a/redisvl/mcp/auth.py +++ b/redisvl/mcp/auth.py @@ -247,7 +247,6 @@ class InjectSpec(Protocol): A protocol rather than an import of the configuration model, so the security core has no dependency on the shape of the YAML that reaches it. - A v1.1 code tool assembles its own specs and gets the same guarantees. """ @property diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index e97f28ef..22daf11d 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -244,7 +244,9 @@ def code_tool( unless ``description`` is given. ``index`` pins the binding (required when several are configured), ``writes`` grants the write methods and requires the write scope, and ``scope`` is an extra scope a caller must - hold on top of the server-wide one. The function is returned unchanged. + hold on top of the server-wide one; like every scope, it is enforced + only when authentication is configured. The function is returned + unchanged. The function is checked here; whether its binding exists and permits it is checked at startup, where the config is known. diff --git a/tests/integration/test_mcp/test_code_tools.py b/tests/integration/test_mcp/test_code_tools.py index cacdf38d..54e15312 100644 --- a/tests/integration/test_mcp/test_code_tools.py +++ b/tests/integration/test_mcp/test_code_tools.py @@ -118,26 +118,6 @@ async def _call(server: RedisVLMCPServer, name: str, **kwargs): return await tool.fn(**kwargs) -async def test_a_read_tool_queries_the_bound_index(server_for): - server = server_for() - - @server.code_tool() - async def open_tickets(ctx: ToolContext, limit: int = 10) -> dict: - """List open support tickets.""" - query = FilterQuery( - filter_expression=Tag("status") == "open", - return_fields=["content"], - num_results=limit, - ) - return {"tickets": sorted(r["content"] for r in await ctx.index.query(query))} - - await server.startup() - - assert await _call(server, "open-tickets") == { - "tickets": ["login failure", "refund delayed"] - } - - async def test_a_writing_tool_writes_through_its_handle(server_for): server = server_for() From 2ad118db42e32871f8bb1e52cefaaf57d6909e44 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 29 Oct 2026 14:53:59 +0100 Subject: [PATCH 10/15] docs(mcp): a writing code tool needs the read scope as well --- docs/concepts/mcp.md | 2 +- docs/user_guide/how_to_guides/mcp.md | 2 +- redisvl/mcp/server.py | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 54dd1cc3..93b9f865 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -309,7 +309,7 @@ The server applies the same rails as for a built-in, without the author asking f - Every `ctx.index` call takes a slot under the server-wide concurrency cap and is bounded by the binding's `runtime.request_timeout_seconds`. The cap is taken per call, so a tool that queries several times never blocks itself. - The whole tool call is bounded by the same timeout, so a tool cannot hold a request open indefinitely. A timeout returns a retryable `backend_unavailable` error. -- A caller needs the server's read scope, or the write scope for a tool declared with `writes=True`. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. Like every scope, it is enforced only when the server has authentication configured. +- A caller needs the server's read scope, and the write scope as well for a tool declared with `writes=True`, whose handle can both read and write. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. Like every scope, it is enforced only when the server has authentication configured. - An exception the tool raises becomes a stable MCP error: `ValueError` becomes `invalid_request`, a Redis error a retryable `backend_unavailable`, most other exceptions `internal_error`. Raise `RedisVLMCPError` to choose the code and message yourself. The handle is a guardrail, not a sandbox. A code tool is your own code, running with the privileges of the process that started the server, and Python can still reach private attributes. What the handle does is make the bounded, policy-checked path the only obvious one. diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 265c130a..6ecc2df2 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -453,7 +453,7 @@ server.run(transport="streamable-http", host="127.0.0.1", port=8000) What the client sees: - `open-tickets-for-product`, taking `product` (required) and `limit`, described by its docstring. -- `delete-spam-ticket`, taking `ticket_id`. With authentication configured, a caller needs the server's write scope and `tickets.triage`; without it, no scope is checked. Either way, the tool deletes nothing that is not already marked as spam. +- `delete-spam-ticket`, taking `ticket_id`. With authentication configured, a caller needs the server's read and write scopes and `tickets.triage`; without it, no scope is checked. Either way, the tool deletes nothing that is not already marked as spam. - No argument for the context or the index, in either tool. Rules worth knowing: diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 22daf11d..36125871 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -243,10 +243,10 @@ def code_tool( the model's arguments after it; its docstring is the tool description unless ``description`` is given. ``index`` pins the binding (required when several are configured), ``writes`` grants the write methods and - requires the write scope, and ``scope`` is an extra scope a caller must - hold on top of the server-wide one; like every scope, it is enforced - only when authentication is configured. The function is returned - unchanged. + requires the write scope on top of the read scope, and ``scope`` is an + extra scope a caller must hold on top of the server-wide ones; like + every scope, it is enforced only when authentication is configured. + The function is returned unchanged. The function is checked here; whether its binding exists and permits it is checked at startup, where the config is known. From 4cbe8d1b03163e7951f51ada38242606950e6025 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 09:44:30 +0100 Subject: [PATCH 11/15] docs(mcp): document load's ids and document_id, with an end-to-end round trip --- docs/concepts/mcp.md | 4 +-- docs/user_guide/how_to_guides/mcp.md | 2 +- tests/integration/test_mcp/test_code_tools.py | 25 +++++++++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 93b9f865..9f7e00b2 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -296,12 +296,12 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | Attribute | What it is | |---|---| -| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. | +| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. `document_id(key)` turns the full key a search result carries as `id` back into a document id. | | `ctx.schema` | The bound index's schema. | | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | | `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the request has no token. | -Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart, and an index with an empty prefix covers every key, so its document ids are full Redis keys. +Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, `load` returns document ids, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart, and an index with an empty prefix covers every key, so its document ids are full Redis keys. Everything else on the index is out of reach: creating, dropping or clearing it, `info` for another index, `paginate`, and the filter-wide bulk writes `update_by_filter` and `drop_by_filter`, which a request timeout could cancel partway through. diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 6ecc2df2..6c5452c2 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -458,7 +458,7 @@ What the client sees: Rules worth knowing: -- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. - **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods. Both types are exported from `redisvl.mcp`. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. diff --git a/tests/integration/test_mcp/test_code_tools.py b/tests/integration/test_mcp/test_code_tools.py index 54e15312..fd61be67 100644 --- a/tests/integration/test_mcp/test_code_tools.py +++ b/tests/integration/test_mcp/test_code_tools.py @@ -133,6 +133,31 @@ async def close_ticket(ctx: ToolContext, ticket_id: str) -> dict: assert await _call(server, "close-ticket", ticket_id="t1") == {"status": "resolved"} +async def test_ids_from_a_load_and_a_search_address_the_same_record(server_for): + # Both return full keys underneath, and the writes re-apply the prefix, so + # only document ids reach the record. + server = server_for() + + @server.code_tool(writes=True) + async def file_and_withdraw(ctx: ToolContext) -> dict: + """File a ticket under a generated id, find it again, then withdraw it.""" + [filed] = await ctx.index.load( + [{"content": "charged twice", "status": "draft"}] + ) + [hit] = await ctx.index.query( + FilterQuery(filter_expression=Tag("status") == "draft") + ) + found = ctx.index.document_id(hit["id"]) + return { + "same": found == filed, + "dropped": await ctx.index.drop_documents(found), + } + + await server.startup() + + assert await _call(server, "file-and-withdraw") == {"same": True, "dropped": 1} + + async def test_a_code_tool_on_a_tenant_scoped_index_fails_startup(server_for): key = RSAKeyPair.generate() server = server_for( From e44db9822721e4737e3db598d56a713ee22be4f0 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 09:58:40 +0100 Subject: [PATCH 12/15] fix(mcp): refuse code tools registered while the server is starting, and cast in the writing example --- docs/user_guide/how_to_guides/mcp.md | 11 +++++++---- redisvl/mcp/server.py | 7 ++++++- tests/unit/test_mcp/test_server_unit.py | 11 +++++++++-- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 6c5452c2..24d915ee 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -420,7 +420,9 @@ The server refuses to start an injecting profile without authentication, over `s When a tool needs logic a profile cannot express, write it as an async function and register it on the server before you run it. This suits a server you start from your own Python process: ```python -from redisvl.mcp import MCPSettings, RedisVLMCPServer, ToolContext +from typing import cast + +from redisvl.mcp import MCPSettings, RedisVLMCPServer, ToolContext, WritableIndexHandle from redisvl.query import FilterQuery from redisvl.query.filter import Tag @@ -441,10 +443,11 @@ async def open_tickets_for_product(ctx: ToolContext, product: str, limit: int = @server.code_tool(index="tickets", writes=True, scope="tickets.triage") async def delete_spam_ticket(ctx: ToolContext, ticket_id: str) -> dict: """Delete a support ticket that triage has marked as spam.""" - ticket = await ctx.index.fetch(ticket_id) + index = cast(WritableIndexHandle, ctx.index) + ticket = await index.fetch(ticket_id) if ticket is None or ticket.get("status") != "spam": raise ValueError(f"ticket {ticket_id} is not marked as spam") - return {"deleted": await ctx.index.drop_documents(ticket_id)} + return {"deleted": await index.drop_documents(ticket_id)} server.run(transport="streamable-http", host="127.0.0.1", port=8000) @@ -459,7 +462,7 @@ What the client sees: Rules worth knowing: - **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. -- **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods. Both types are exported from `redisvl.mcp`. +- **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods, as `delete-spam-ticket` shows. Both types are exported from `redisvl.mcp`. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. - **Register before starting.** Tools register once, at the first startup, so a tool registered afterwards is refused. diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 36125871..8a575071 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -253,7 +253,12 @@ def code_tool( """ def decorator(fn: _ToolFn) -> _ToolFn: - if self._tools_registered: + # While starting, the code tools have already been read, so one + # added now would be silently left out. + if ( + self._tools_registered + or self._lifecycle_state is _LifecycleState.STARTING + ): raise RuntimeError( "MCP tools are registered once, at the first startup; " "register code tools before starting the server" diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index cbde2416..9120187f 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -1067,6 +1067,7 @@ def _code_tool_server(*, read_only=False) -> RedisVLMCPServer: server = RedisVLMCPServer.__new__(RedisVLMCPServer) server._code_tools = [] server._tools_registered = False + server._lifecycle_state = server._LifecycleState.INITIAL server.mcp_settings = SimpleNamespace(read_only=read_only) return server @@ -1117,12 +1118,18 @@ def sync_tool(ctx: ToolContext, query: str) -> dict: assert server._code_tools == [] -def test_custom_tool_after_registration_is_refused(): +@pytest.mark.parametrize("moment", ["registered", "starting"]) +def test_custom_tool_after_registration_is_refused(moment): server = _code_tool_server() - server._tools_registered = True + if moment == "registered": + server._tools_registered = True + else: + # Startup has read the code tools but not yet registered them. + server._lifecycle_state = server._LifecycleState.STARTING with pytest.raises(RuntimeError, match="register code tools before starting"): server.code_tool()(recent_tickets) + assert server._code_tools == [] def test_a_code_tool_on_a_single_index_server_binds_to_it(): From 4da4dc197f573fad64fbb8770993d34ee0916bb6 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 10:32:09 +0100 Subject: [PATCH 13/15] docs(mcp): ctx.claims is None on a server without authentication --- docs/concepts/mcp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 9f7e00b2..9f67f627 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -299,7 +299,7 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. `document_id(key)` turns the full key a search result carries as `id` back into a document id. | | `ctx.schema` | The bound index's schema. | | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | -| `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the request has no token. | +| `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the server has no authentication or the request has no token. | Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, `load` returns document ids, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart, and an index with an empty prefix covers every key, so its document ids are full Redis keys. From 73a62d806e32a5cf7f19fea46e6334d96c23b1ba Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 11:02:39 +0100 Subject: [PATCH 14/15] docs(mcp): a JSON FilterQuery needs return_fields to keep result keys --- docs/concepts/mcp.md | 2 +- docs/user_guide/how_to_guides/mcp.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 9f67f627..cf225b8b 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -296,7 +296,7 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | Attribute | What it is | |---|---| -| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. `document_id(key)` turns the full key a search result carries as `id` back into a document id. | +| `ctx.index` | A handle on the bound index offering `query`, `batch_query`, `search`, `batch_search`, `aggregate`, `fetch`, `exists` and `info`. A tool declared with `writes=True` also gets `load`, `drop_documents` and `expire_documents`. `document_id(key)` turns the full key a search result carries as `id` back into a document id. On JSON storage, a `FilterQuery` without `return_fields` returns whole documents, whose own `id` field replaces the key, so set `return_fields` when you need it. | | `ctx.schema` | The bound index's schema. | | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | | `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the server has no authentication or the request has no token. | diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 24d915ee..0078b4b5 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -461,7 +461,7 @@ What the client sees: Rules worth knowing: -- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. On JSON storage, give a `FilterQuery` `return_fields`, or a document's own `id` field replaces the key in its results. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. - **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods, as `delete-spam-ticket` shows. Both types are exported from `redisvl.mcp`. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. From 6f00412dddad0142d6ddf3bc1a67701a6e883cd6 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 30 Oct 2026 13:18:45 +0100 Subject: [PATCH 15/15] docs(mcp): document restarts, load errors and prefix limits for code tools; test the recorded pairing --- docs/concepts/mcp.md | 6 ++-- docs/user_guide/how_to_guides/mcp.md | 4 +-- tests/unit/test_mcp/test_server_unit.py | 37 +++++++++++++++---------- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index cf225b8b..f879f4a5 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -250,7 +250,7 @@ Injection is checked at startup wherever the configuration alone can show it wou An injected field must be a tag. Text equality is a phrase match over tokenised text, and text is tokenised on punctuation, so the phrase `acme-corp` would also match `acme-corp-eu`. -The tool set registers once per process. If a restart reloads a configuration that differs from the registered one, the server normally logs a warning and keeps the old tools. When injection is configured on either side of the change, startup fails instead, because keeping the old tenant scoping in force is not something a log line should report. +The tool set registers once per process. If a restart reloads a configuration that differs from the registered one, the server normally logs a warning and keeps the old tools. When injection is configured on either side of the change, startup fails instead, because keeping the old tenant scoping in force is not something a log line should report. Startup also fails when the change alters the code tools or the bindings they resolve to, since operator code would otherwise keep running against the old pairing. #### Threat Model @@ -301,7 +301,7 @@ A code tool never receives the server, the real `AsyncSearchIndex`, or a Redis c | `ctx.vectorizer` and `ctx.embed(text)` | The binding's vectorizer, if it has one, and a helper that embeds text with it. | | `ctx.claims` | A read-only copy of the current request's verified token claims, or `None` when the server has no authentication or the request has no token. | -Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, `load` returns document ids, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart, and an index with an empty prefix covers every key, so its document ids are full Redis keys. +Every call stays inside the bound index. Searches and aggregations run against its name, and records are addressed by document id, which the index turns into a key under its own prefix. So nothing on the handle takes a full Redis key, `load` returns document ids, and `query` refuses a `SQLQuery`, whose `FROM` clause names its own index. Either could otherwise reach another index's documents, including a tenant-scoped one. An index whose key prefix overlaps another's shares keys with it, which the handle cannot tell apart. A prefix that is empty, or made only of separator characters such as `:`, turns document ids into full Redis keys, so every key is in reach. On an index with several prefixes, document ids address records under the first one only. Everything else on the index is out of reach: creating, dropping or clearing it, `info` for another index, `paginate`, and the filter-wide bulk writes `update_by_filter` and `drop_by_filter`, which a request timeout could cancel partway through. @@ -310,7 +310,7 @@ The server applies the same rails as for a built-in, without the author asking f - Every `ctx.index` call takes a slot under the server-wide concurrency cap and is bounded by the binding's `runtime.request_timeout_seconds`. The cap is taken per call, so a tool that queries several times never blocks itself. - The whole tool call is bounded by the same timeout, so a tool cannot hold a request open indefinitely. A timeout returns a retryable `backend_unavailable` error. - A caller needs the server's read scope, and the write scope as well for a tool declared with `writes=True`, whose handle can both read and write. A tool can require an extra scope with `scope=`, which a caller must hold as well, so a code tool can be stricter than the built-ins but never looser. Like every scope, it is enforced only when the server has authentication configured. -- An exception the tool raises becomes a stable MCP error: `ValueError` becomes `invalid_request`, a Redis error a retryable `backend_unavailable`, most other exceptions `internal_error`. Raise `RedisVLMCPError` to choose the code and message yourself. +- An exception the tool raises becomes a stable MCP error: `ValueError` becomes `invalid_request`, a Redis error from a search, aggregation or fetch a retryable `backend_unavailable`, and most other exceptions `internal_error`. That includes any failure of `ctx.index.load`, which reports even a lost connection as a non-retryable `internal_error`. Raise `RedisVLMCPError` to choose the code and message yourself. The handle is a guardrail, not a sandbox. A code tool is your own code, running with the privileges of the process that started the server, and Python can still reach private attributes. What the handle does is make the bounded, policy-checked path the only obvious one. diff --git a/docs/user_guide/how_to_guides/mcp.md b/docs/user_guide/how_to_guides/mcp.md index 0078b4b5..7a918013 100644 --- a/docs/user_guide/how_to_guides/mcp.md +++ b/docs/user_guide/how_to_guides/mcp.md @@ -461,7 +461,7 @@ What the client sees: Rules worth knowing: -- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. On JSON storage, give a `FilterQuery` `return_fields`, or a document's own `id` field replaces the key in its results. That does not separate indexes whose prefixes overlap, or an index with an empty prefix, which covers every key. There is no Redis client and no way to create, drop or clear the index. +- **A code tool reaches Redis only through `ctx.index`.** It has the read methods, plus a handful of bounded writes for a tool registered with `writes=True`. Records are addressed by document id, never by full Redis key, so a tool stays under its index's key prefix. That does not separate indexes whose prefixes overlap, and a prefix that is empty or made only of separator characters covers every key. On an index with several prefixes, ids address the first prefix only. `load` returns document ids, and `ctx.index.document_id(hit["id"])` turns a search result's key into one. On JSON storage, give a `FilterQuery` `return_fields`, or a document's own `id` field replaces the key in its results. There is no Redis client and no way to create, drop or clear the index. - **Writing tools need a cast for type checkers.** `ctx.index` is typed as `IndexHandle`. In a tool registered with `writes=True`, `cast(WritableIndexHandle, ctx.index)` gives a type checker the write methods, as `delete-spam-ticket` shows. Both types are exported from `redisvl.mcp`. - **Every call is bounded.** Each `ctx.index` call takes a slot under the concurrency cap and the binding's `request_timeout_seconds`, and the whole tool call is bounded by the same timeout. - **Raise to report a problem.** A `ValueError` reaches the client as `invalid_request`; raise `RedisVLMCPError` to choose the error code yourself. @@ -865,4 +865,4 @@ The token is verified but its claim is unusable: missing, empty, padded with whi ### Code Tool Fails at Startup -The error names the tool. Either its `index` is missing while several indexes are configured or names no configured binding, or another custom tool already uses its name, or it is registered with `writes=True` on a binding or server that is read-only. A code tool registered after the server has started is refused with a `RuntimeError`; register every tool before calling `run`, `run_async` or `startup`. +The error names the tool. Either its `index` is missing while several indexes are configured or names no configured binding, or another custom tool already uses its name, or it is registered with `writes=True` on a binding or server that is read-only. A code tool registered after the server has started is refused with a `RuntimeError`; register every tool before calling `run`, `run_async` or `startup`. A restart in the same process also fails with a `RuntimeError` if the code tools, or the indexes they bind to, have changed; restart the process instead. diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 9120187f..23999d50 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -152,6 +152,17 @@ def _register_tools_with( monkeypatch, bindings: dict, *, config=None, code_tools=() ) -> list[str]: """Run _register_tools against the given bindings, returning registered names.""" + server, registered = _registration_server( + monkeypatch, bindings, config=config, code_tools=code_tools + ) + server._register_tools() + return registered + + +def _registration_server( + monkeypatch, bindings: dict, *, config=None, code_tools=() +) -> tuple[RedisVLMCPServer, list[str]]: + """A server ready for _register_tools, recording every name it registers.""" registered: list[str] = [] monkeypatch.setattr( "redisvl.mcp.server.register_code_tool", @@ -194,9 +205,7 @@ def fake_register_profile_tools(server): server.config = config server.mcp_settings = SimpleNamespace(read_only=False) server._resolved_code_tools = tuple(code_tools) - - server._register_tools() - return registered + return server, registered def _config_with(*, builtin_tools=None, custom_tools=None) -> MCPConfig: @@ -1284,24 +1293,22 @@ def test_register_tools_registers_code_tools_after_profiles(monkeypatch): assert registered[-2:] == ["resolved-search", "recent-tickets"] -def _restart_with(*, registered_on, restarted_on): +def _restart_with(monkeypatch, *, registered_on, restarted_on): """Register a code tool on one binding, then restart pairing it with another.""" spec = build_code_tool_spec(recent_tickets) - server = RedisVLMCPServer.__new__(RedisVLMCPServer) - server._tools_registered = True - server._registered_code_tools = ((spec, registered_on),) + # The first startup records the pairing itself, as a real server does. + server, _ = _registration_server( + monkeypatch, {}, code_tools=[(spec, registered_on)] + ) + server._register_tools() server._resolved_code_tools = ((spec, restarted_on),) - server._registered_tool_fingerprint = server._tool_surface_fingerprint(None) - server._registered_tools_inject = False - server.tool = object() - server.config = None server._register_tools() -def test_a_restart_that_moves_a_code_tool_is_fatal(): +def test_a_restart_that_moves_a_code_tool_is_fatal(monkeypatch): with pytest.raises(RuntimeError, match="code tools, or the indexes they are"): - _restart_with(registered_on="knowledge", restarted_on="tickets") + _restart_with(monkeypatch, registered_on="knowledge", restarted_on="tickets") -def test_a_restart_with_the_same_code_tools_proceeds(): - _restart_with(registered_on="knowledge", restarted_on="knowledge") +def test_a_restart_with_the_same_code_tools_proceeds(monkeypatch): + _restart_with(monkeypatch, registered_on="knowledge", restarted_on="knowledge")