From 04fbfaddd67c445720a9db56581052c3534990e1 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 10:41:44 +0200 Subject: [PATCH 1/4] feat(mcp): let operators disable individual built-in tools `list-indexes`, `search-records`, and `upsert-records` all registered unconditionally, so an operator who wanted a narrower tool surface had no way to get one -- a deployment that should never advertise writes still published `upsert-records` whenever any binding was writable, and a single-purpose server still published discovery. `server.builtin_tools` maps a built-in name to enabled/disabled. Omitted names stay enabled, so existing configs are unaffected. Only the three real names are accepted: `search_records` with underscores fails at startup rather than silently disabling nothing while reading as though it had. Two tool-set shapes are valid config but unusable in practice, and both would otherwise be silent. Disabling everything leaves a server that connects and offers nothing. Disabling discovery on a multi-index server leaves `search-records` demanding a logical index id that clients have no way to learn, since its own description tells them to call `list-indexes` first. Both now log a warning at startup; neither is fatal, because an operator may be mid-rollout. The empty-surface warning deliberately does not name a cause, since `upsert-records` can also be absent because every binding is read-only. The multi-index discovery case had no coverage, so this adds it along with the negative case: a sole binding makes the index argument default, so disabling discovery there is legitimate and stays quiet. --- docs/concepts/mcp.md | 16 +++- redisvl/mcp/config.py | 31 +++++++ redisvl/mcp/server.py | 61 ++++++++++++- redisvl/mcp/tools/list_indexes.py | 6 +- tests/unit/test_mcp/test_config.py | 62 ++++++++++++- tests/unit/test_mcp/test_server.py | 7 +- tests/unit/test_mcp/test_server_unit.py | 113 +++++++++++++++++++++++- 7 files changed, 285 insertions(+), 11 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index ff192d266..97954a30b 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -86,7 +86,7 @@ MCP-reserved score metadata field names for the configured search mode. ## Read-Only and Read-Write Modes -RedisVL MCP always registers `search-records` and `list-indexes`. +RedisVL MCP registers `search-records` and `list-indexes` by default (see [Tool Surface](#tool-surface) for turning a built-in off deliberately). Write availability is enforced at two levels: @@ -105,12 +105,22 @@ For configuration and the gateway boundary, see {doc}`/user_guide/how_to_guides/ ## Tool Surface -RedisVL MCP exposes up to three tools: +RedisVL MCP exposes up to three built-in tools: -- `list-indexes` enumerates the configured logical indexes for discovery (always available) +- `list-indexes` enumerates the configured logical indexes for discovery - `search-records` searches a selected index using that index's server-owned search mode - `upsert-records` validates and upserts records into a selected writable index, embedding them only when that capability is configured +Any of the three can be turned off with `server.builtin_tools` — useful for a server that should only ever read, or one that should not advertise discovery: + +```yaml +server: + builtin_tools: + upsert-records: disabled +``` + +Only the three names above are accepted; anything else fails at startup rather than being silently ignored. A server whose tool set ends up unusable — no tools at all, or discovery disabled on a multi-index server, where clients cannot learn the logical index ids `search-records` requires — logs a warning at startup. + These tools follow a stable contract: - request validation happens before query or write execution diff --git a/redisvl/mcp/config.py b/redisvl/mcp/config.py index 8bafcc5d7..f40d7f103 100644 --- a/redisvl/mcp/config.py +++ b/redisvl/mcp/config.py @@ -26,11 +26,23 @@ ) +_BUILTIN_TOOL_NAMES = frozenset({"list-indexes", "search-records", "upsert-records"}) + + def reserved_score_metadata_field_names() -> frozenset[str]: """Return MCP-reserved score metadata field names.""" return _RESERVED_SCORE_METADATA_FIELDS +def builtin_tool_names() -> frozenset[str]: + """Return the names of the built-in MCP tools. + + These register by default and can be turned off individually through + ``server.builtin_tools``, so they are not unconditionally available. + """ + return _BUILTIN_TOOL_NAMES + + class MCPRuntimeConfig(BaseModel): """Runtime limits and validated field mappings for MCP requests.""" @@ -200,6 +212,25 @@ class MCPServerConfig(BaseModel): redis_url: str = Field(..., min_length=1) auth: MCPAuthConfig | None = None transport_security: MCPTransportSecurityConfig | None = None + builtin_tools: dict[str, Literal["enabled", "disabled"]] = Field( + default_factory=dict + ) + + @model_validator(mode="after") + def _validate_builtin_tools(self) -> "MCPServerConfig": + """Reject disable/enable entries that do not name a built-in tool.""" + unknown = sorted(set(self.builtin_tools) - builtin_tool_names()) + if unknown: + raise ValueError( + "server.builtin_tools contains unknown tool names: " + f"{', '.join(unknown)}; known built-ins: " + f"{', '.join(sorted(builtin_tool_names()))}" + ) + return self + + def builtin_tool_enabled(self, tool_name: str) -> bool: + """Report whether a built-in tool should be registered.""" + return self.builtin_tools.get(tool_name, "enabled") == "enabled" class MCPIndexSearchConfig(BaseModel): diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 271983a94..d053d7b02 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -282,17 +282,70 @@ def _register_tools(self) -> None: if len(self._bindings) == 1: search_schema = next(iter(self._bindings.values())).schema - # Discovery is always available so clients can enumerate indexes. - register_list_indexes_tool(self) - register_search_tool(self, search_schema) + # An operator can turn off a built-in whose capability the server should + # not offer at all -- a read-only deployment, or one that should not + # advertise discovery. + config = getattr(self, "config", None) + enabled = ( + config.server.builtin_tool_enabled + if config is not None + else lambda _name: True + ) + + registered: list[str] = [] + + # Discovery is on by default so clients can enumerate indexes. + if enabled("list-indexes"): + register_list_indexes_tool(self) + registered.append("list-indexes") + if enabled("search-records"): + register_search_tool(self, search_schema) + registered.append("search-records") # Expose upsert only when at least one binding is writable. A binding is # read-only under global read-only mode or its own read_only policy, both # of which are folded into effective_read_only; the per-call write check # in the tool then rejects writes to any individual read-only binding. - if any(not rt.effective_read_only for rt in self._bindings.values()): + if enabled("upsert-records") and any( + not rt.effective_read_only for rt in self._bindings.values() + ): register_upsert_tool(self) + registered.append("upsert-records") + + self._warn_on_unusable_tool_surface(registered) self._tools_registered = True + def _warn_on_unusable_tool_surface(self, registered: list[str]) -> None: + """Warn about tool-set shapes that are valid config but unusable in practice. + + Neither case is fatal -- an operator may be mid-rollout -- but both are + silent otherwise, and both present to a client as a server that simply + does not work. + """ + if not registered: + # Deliberately does not attribute a cause: `upsert-records` can also + # be absent because every binding is read-only, not because + # `builtin_tools` disabled it. + logger.warning( + "MCP server registered no tools, so clients will see an empty " + "tool list. Check server.builtin_tools and read-only settings." + ) + return + + # `search-records`'s multi-index description tells clients to call + # list-indexes first, so disabling discovery leaves them unable to learn + # the logical ids the tool requires. + if ( + len(self._bindings) > 1 + and "search-records" in registered + and "list-indexes" not in registered + ): + logger.warning( + "MCP server has %d indexes and exposes search-records, but " + "list-indexes is disabled: clients cannot discover the logical " + "index ids that search-records requires.", + len(self._bindings), + ) + @asynccontextmanager async def _server_lifespan(self, _server: Any): """Bridge FastMCP lifespan hooks onto the server's explicit lifecycle.""" diff --git a/redisvl/mcp/tools/list_indexes.py b/redisvl/mcp/tools/list_indexes.py index 58a6adefc..ede731ab4 100644 --- a/redisvl/mcp/tools/list_indexes.py +++ b/redisvl/mcp/tools/list_indexes.py @@ -82,7 +82,11 @@ def list_indexes(server: "RedisVLMCPServer") -> dict[str, Any]: def register_list_indexes_tool(server: "RedisVLMCPServer") -> None: - """Register the always-available, read-only `list-indexes` MCP tool.""" + """Register the read-only `list-indexes` MCP tool. + + Registered by default; an operator can turn it off through + ``server.builtin_tools``. + """ async def list_indexes_tool(): """FastMCP wrapper for the `list-indexes` tool.""" diff --git a/tests/unit/test_mcp/test_config.py b/tests/unit/test_mcp/test_config.py index 2a738d875..57cdd3bf0 100644 --- a/tests/unit/test_mcp/test_config.py +++ b/tests/unit/test_mcp/test_config.py @@ -4,7 +4,7 @@ import pytest import yaml -from redisvl.mcp.config import MCPConfig, load_mcp_config +from redisvl.mcp.config import MCPConfig, builtin_tool_names, load_mcp_config from redisvl.schema import IndexSchema @@ -558,3 +558,63 @@ def test_mcp_config_still_accepts_a_real_text_field(): schema = binding.to_index_schema(_inspected_schema()) binding.validate_runtime_mapping(schema) + + +def test_mcp_config_builtin_tools_default_to_enabled(): + config = MCPConfig.model_validate(_valid_config()) + + assert config.server.builtin_tools == {} + for tool_name in builtin_tool_names(): + assert config.server.builtin_tool_enabled(tool_name) is True + + +def test_mcp_config_builtin_tools_can_disable_a_builtin(): + config = _valid_config() + config["server"]["builtin_tools"] = { + "search-records": "disabled", + "list-indexes": "enabled", + } + + loaded = MCPConfig.model_validate(config) + + assert loaded.server.builtin_tool_enabled("search-records") is False + assert loaded.server.builtin_tool_enabled("list-indexes") is True + # Unmentioned built-ins stay enabled. + assert loaded.server.builtin_tool_enabled("upsert-records") is True + + +def test_mcp_config_rejects_unknown_builtin_tool_names(): + config = _valid_config() + # Underscores instead of hyphens -- the most likely typo, and one that would + # otherwise disable nothing while reading as though it had. + config["server"]["builtin_tools"] = {"search_records": "disabled"} + + with pytest.raises( + ValueError, match="server.builtin_tools contains unknown tool names" + ): + MCPConfig.model_validate(config) + + +def test_load_mcp_config_parses_builtin_tools_from_yaml(tmp_path: Path): + config_path = tmp_path / "mcp.yaml" + config_path.write_text( + """ +server: + redis_url: redis://localhost:6379 + builtin_tools: + upsert-records: disabled +indexes: + knowledge: + redis_name: docs-index + search: + type: fulltext + runtime: + text_field_name: content +""".strip(), + encoding="utf-8", + ) + + config = load_mcp_config(str(config_path)) + + assert config.server.builtin_tool_enabled("upsert-records") is False + assert config.server.builtin_tool_enabled("search-records") is True diff --git a/tests/unit/test_mcp/test_server.py b/tests/unit/test_mcp/test_server.py index 9179bdd5b..ddabde013 100644 --- a/tests/unit/test_mcp/test_server.py +++ b/tests/unit/test_mcp/test_server.py @@ -59,7 +59,12 @@ def _binding_namespace( def _startup_config(indexes=None): return SimpleNamespace( - server=SimpleNamespace(redis_url="redis://localhost:6379"), + server=SimpleNamespace( + redis_url="redis://localhost:6379", + # Mirrors MCPServerConfig: every built-in is enabled unless a config + # explicitly disables it. + builtin_tool_enabled=lambda _name: True, + ), indexes=indexes or {"knowledge": _binding_namespace()}, ) diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 0bd580d98..9f3c8f2f2 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -1,7 +1,9 @@ +import logging from types import SimpleNamespace import pytest +from redisvl.mcp.config import MCPConfig, builtin_tool_names from redisvl.mcp.errors import MCPErrorCode, RedisVLMCPError from redisvl.mcp.runtime import BindingRuntime from redisvl.mcp.server import RedisVLMCPServer @@ -144,7 +146,7 @@ async def fake_close_resources(self, *, index, vectorizer): assert server._tools_registered is True -def _register_tools_with(monkeypatch, bindings: dict) -> list[str]: +def _register_tools_with(monkeypatch, bindings: dict, *, config=None) -> list[str]: """Run _register_tools against the given bindings, returning registered names.""" registered: list[str] = [] monkeypatch.setattr( @@ -164,12 +166,32 @@ def _register_tools_with(monkeypatch, bindings: dict) -> list[str]: server._bindings = bindings server._tools_registered = False server.tool = object() + server.config = config server.mcp_settings = SimpleNamespace(read_only=False) server._register_tools() return registered +def _config_with(*, builtin_tools=None) -> MCPConfig: + """Build a real validated config so the gating logic sees the real methods.""" + server_config: dict = {"redis_url": "redis://localhost:6379"} + if builtin_tools is not None: + server_config["builtin_tools"] = builtin_tools + return MCPConfig.model_validate( + { + "server": server_config, + "indexes": { + "knowledge": { + "redis_name": "docs-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + } + }, + } + ) + + def test_register_tools_exposes_upsert_when_a_binding_is_writable(monkeypatch): registered = _register_tools_with( monkeypatch, @@ -197,3 +219,92 @@ def test_register_tools_hides_upsert_when_every_binding_is_read_only(monkeypatch # Read paths stay available even when writes are globally disabled. assert "list-indexes" in registered assert "search-records" in registered + + +def test_register_tools_registers_every_builtin_when_no_config_is_attached(monkeypatch): + registered = _register_tools_with( + monkeypatch, {"knowledge": _binding_runtime("knowledge")} + ) + + assert registered == ["list-indexes", "search-records", "upsert-records"] + + +@pytest.mark.parametrize( + "disabled_tool", ["list-indexes", "search-records", "upsert-records"] +) +def test_register_tools_skips_a_builtin_the_operator_disabled( + monkeypatch, disabled_tool +): + registered = _register_tools_with( + monkeypatch, + {"knowledge": _binding_runtime("knowledge")}, + config=_config_with(builtin_tools={disabled_tool: "disabled"}), + ) + + assert disabled_tool not in registered + # Disabling one built-in must not take the others with it. + for other in {"list-indexes", "search-records", "upsert-records"} - {disabled_tool}: + assert other in registered + + +def test_register_tools_warns_when_the_whole_tool_surface_is_empty(monkeypatch, caplog): + """Every built-in disabled is valid config but a dead server.""" + with caplog.at_level(logging.WARNING, logger="redisvl.mcp.server"): + registered = _register_tools_with( + monkeypatch, + {"knowledge": _binding_runtime("knowledge")}, + config=_config_with( + builtin_tools={name: "disabled" for name in builtin_tool_names()} + ), + ) + + # The surface really is empty, so the warning is not passing for some other + # reason. + assert registered == [] + # A client sees a server that connects and then offers nothing, which is + # indistinguishable from a broken deployment unless the operator is told. + assert [ + record.message + for record in caplog.records + if "registered no tools" in record.message + ] + + +def test_register_tools_warns_when_discovery_is_disabled_on_a_multi_index_server( + monkeypatch, caplog +): + """search-records needs logical index ids that only list-indexes reveals.""" + with caplog.at_level(logging.WARNING, logger="redisvl.mcp.server"): + registered = _register_tools_with( + monkeypatch, + { + "knowledge": _binding_runtime("knowledge"), + "tickets": _binding_runtime("tickets"), + }, + config=_config_with(builtin_tools={"list-indexes": "disabled"}), + ) + + assert "search-records" in registered and "list-indexes" not in registered + assert [ + record.message + for record in caplog.records + if "cannot discover" in record.message + ] + + +def test_register_tools_stays_quiet_when_discovery_is_disabled_on_one_index( + monkeypatch, caplog +): + """With a sole binding the index argument defaults, so discovery is optional.""" + with caplog.at_level(logging.WARNING, logger="redisvl.mcp.server"): + _register_tools_with( + monkeypatch, + {"knowledge": _binding_runtime("knowledge")}, + config=_config_with(builtin_tools={"list-indexes": "disabled"}), + ) + + assert not [ + record.message + for record in caplog.records + if "cannot discover" in record.message + ] From 6018b3b497458ffe35db56c601d2172010e4937d Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 13 Aug 2026 10:16:39 +0200 Subject: [PATCH 2/4] fix(mcp): stop the tool surface advertising what it withholds Three review findings, all consequences of making the built-in tool set configurable: the rest of the surface still described the old, always-on one. **`list-indexes` misreported upsert availability.** `upsert_available` came from the binding's read-only state alone, so a writable binding on a server with `upsert-records` disabled reported `true` for a tool the client cannot call. It is a client-facing claim about what is callable, so it now also requires the tool to be published. Before this change the tool was registered whenever any binding was writable, which is why the old derivation was correct and is not any more. **The search description pointed at a missing tool.** On a multi-index server with `list-indexes` disabled, `search-records` still told clients to call it first -- while `index` remains required and the ids are otherwise unlearnable, so the contract was not just wrong but unsatisfiable. The description now names the ids inline in that case. The startup warning stays, because inlining ids is a fallback rather than an endorsement of the shape. **An edited config looked applied when it was not.** Tools register once per process and `_tools_registered` deliberately survives teardown, since re-registering the same names on the FastMCP object is invalid. That was harmless while the tool set derived only from binding state, which is re-derived at startup. Now it depends on config, and `startup()` re-reads that file -- so a stop/start against an edited config silently keeps the old tools. The server fingerprints the config its tools were built from and warns on a mismatch. The dangerous direction is an operator disabling a tool, restarting, and believing it is gone. Each fix has a test that fails when only that fix is reverted, plus a control for the enabled path. Docs updated so the advertised behavior matches. --- docs/concepts/mcp.md | 11 ++- redisvl/mcp/server.py | 36 +++++++- redisvl/mcp/tools/list_indexes.py | 21 ++++- redisvl/mcp/tools/search.py | 26 +++++- .../test_mcp/test_list_indexes_tool_unit.py | 50 ++++++++++ tests/unit/test_mcp/test_server.py | 6 +- tests/unit/test_mcp/test_server_unit.py | 92 ++++++++++++++++++- 7 files changed, 229 insertions(+), 13 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index 97954a30b..e8d572619 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -119,7 +119,16 @@ server: upsert-records: disabled ``` -Only the three names above are accepted; anything else fails at startup rather than being silently ignored. A server whose tool set ends up unusable — no tools at all, or discovery disabled on a multi-index server, where clients cannot learn the logical index ids `search-records` requires — logs a warning at startup. +Only the three names above are accepted; anything else fails at startup rather than being silently ignored. + +Disabling a built-in adjusts what the rest of the surface advertises, so the published contract never points at something the server withholds: + +- `list-indexes` reports `upsert_available: false` for every binding when `upsert-records` is disabled, since a writable binding still cannot be written to through a tool that is not published. +- On a multi-index server with `list-indexes` disabled, `search-records` names the available index ids in its own description instead of telling clients to call a discovery tool that does not exist. That server still logs a startup warning, because inlining the ids is a fallback rather than an endorsement of the shape. + +A server whose tool set ends up unusable — no tools at all, or discovery disabled on a multi-index server — logs a warning at startup. + +Tools register once per process. `builtin_tools` is re-read on restart, but the registered tool set is not rebuilt, so a stop/start against an edited config keeps the previous tools and logs a warning saying so. Start a new process to change the tool surface. These tools follow a stable contract: diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index d053d7b02..349367481 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -68,6 +68,7 @@ def __init__(self, settings: MCPSettings): self._bindings: dict[str, BindingRuntime] = {} self._semaphore: asyncio.Semaphore | None = None self._tools_registered = False + self._registered_tool_fingerprint = "" # Lifecycle management self._lifecycle_state = _LifecycleState.INITIAL # Server lifecycle @@ -270,9 +271,32 @@ async def _probe_native_hybrid_search(index: AsyncSearchIndex) -> bool: return hasattr(client.ft(index.schema.index.name), "hybrid_search") + @staticmethod + def _tool_surface_fingerprint(config: Any) -> str: + """Summarize the config that a registered tool set baked in.""" + if config is None: + return "" + return repr(sorted(config.server.builtin_tools.items())) + def _register_tools(self) -> None: """Register MCP tools once every binding is ready.""" if self._tools_registered or not hasattr(self, "tool"): + # Registration is deliberately once-per-process, since re-registering + # the same names on the FastMCP object is not valid. Built-in tool + # closures resolve their binding per call, so they survive a restart + # unchanged -- but which built-ins exist is now a function of config, + # and `startup()` re-reads that file. A stop/start against an edited + # config therefore keeps the old tool set, and the dangerous direction + # is an operator disabling a tool and believing the restart applied it. + if self._tools_registered: + current = self._tool_surface_fingerprint(getattr(self, "config", None)) + if current != self._registered_tool_fingerprint: + logger.warning( + "MCP built-in tool configuration changed since tools were " + "registered, but tools register once per process. The " + "previously registered tool set is still in effect; " + "restart the process to apply the new configuration." + ) return # The search description advertises schema-specific filter hints, which @@ -299,7 +323,16 @@ def _register_tools(self) -> None: register_list_indexes_tool(self) registered.append("list-indexes") if enabled("search-records"): - register_search_tool(self, search_schema) + # Without discovery the caller cannot learn the logical ids, so the + # description has to name them rather than pointing at a tool that is + # not published. Only relevant when the schema is ambiguous, which is + # exactly the multi-binding case. + unlisted_index_ids = ( + sorted(self._bindings) + if search_schema is None and "list-indexes" not in registered + else None + ) + register_search_tool(self, search_schema, index_ids=unlisted_index_ids) registered.append("search-records") # Expose upsert only when at least one binding is writable. A binding is # read-only under global read-only mode or its own read_only policy, both @@ -312,6 +345,7 @@ def _register_tools(self) -> None: registered.append("upsert-records") self._warn_on_unusable_tool_surface(registered) + self._registered_tool_fingerprint = self._tool_surface_fingerprint(config) self._tools_registered = True def _warn_on_unusable_tool_surface(self, registered: list[str]) -> None: diff --git a/redisvl/mcp/tools/list_indexes.py b/redisvl/mcp/tools/list_indexes.py index ede731ab4..261be7c58 100644 --- a/redisvl/mcp/tools/list_indexes.py +++ b/redisvl/mcp/tools/list_indexes.py @@ -49,13 +49,20 @@ def _binding_limits(binding_runtime: BindingRuntime) -> dict[str, int]: } -def _describe_binding(binding_runtime: BindingRuntime) -> dict[str, Any]: +def _describe_binding( + binding_runtime: BindingRuntime, *, upsert_tool_available: bool = True +) -> dict[str, Any]: """Build the deterministic discovery payload for a single binding.""" entry: dict[str, Any] = {"id": binding_runtime.binding_id} if binding_runtime.binding.description is not None: entry["description"] = binding_runtime.binding.description - # Reflects both global read-only and the per-index read_only policy. - entry["upsert_available"] = not binding_runtime.effective_read_only + # Reflects global read-only, the per-index read_only policy, and whether the + # tool is published at all. A writable binding on a server that disabled + # `upsert-records` still cannot be written to, so reporting availability from + # read-only state alone would advertise a tool the client cannot call. + entry["upsert_available"] = ( + upsert_tool_available and not binding_runtime.effective_read_only + ) entry["fields"] = _binding_fields(binding_runtime) limits = _binding_limits(binding_runtime) if limits: @@ -73,9 +80,15 @@ def list_indexes(server: "RedisVLMCPServer") -> dict[str, Any]: # client could misread as "no indexes configured". if not server._bindings: raise RuntimeError("MCP server has not been started") + config = getattr(server, "config", None) + upsert_tool_available = config is None or config.server.builtin_tool_enabled( + "upsert-records" + ) return { "indexes": [ - _describe_binding(binding_runtime) + _describe_binding( + binding_runtime, upsert_tool_available=upsert_tool_available + ) for binding_runtime in server._bindings.values() ], } diff --git a/redisvl/mcp/tools/search.py b/redisvl/mcp/tools/search.py index 3334b3471..bbd3a2ff8 100644 --- a/redisvl/mcp/tools/search.py +++ b/redisvl/mcp/tools/search.py @@ -51,16 +51,31 @@ def _build_return_fields_hint(schema: IndexSchema) -> str: def _build_search_tool_description( - schema: IndexSchema | None, base_description: str | None = None + schema: IndexSchema | None, + base_description: str | None = None, + *, + index_ids: list[str] | None = None, ) -> str: """Build the `search-records` description from static text plus schema hints. With multiple bindings configured the schema is ambiguous (the caller picks - an index per call via `list-indexes`), so per-field hints are omitted and a - routing note is appended instead. + an index per call), so per-field hints are omitted and a routing note is + appended instead. + + ``index_ids`` is supplied only when discovery is unavailable -- an operator + can disable ``list-indexes``, and pointing clients at a tool the server does + not publish would leave them unable to satisfy the required ``index`` + argument at all. Naming the ids inline is the only way they can learn them. """ description = (base_description or DEFAULT_SEARCH_DESCRIPTION).strip() if schema is None: + if index_ids: + return ( + description + " Multiple indexes are configured and discovery is " + "disabled: pass one of these index ids as the `index` argument: " + + ", ".join(index_ids) + + "." + ) return ( description + " Multiple indexes are configured: call list-indexes " "first, then pass the chosen index id as the `index` argument." @@ -498,9 +513,12 @@ async def search_records( raise map_exception(exc) from exc -def register_search_tool(server: Any, schema: IndexSchema | None) -> None: +def register_search_tool( + server: Any, schema: IndexSchema | None, *, index_ids: list[str] | None = None +) -> None: """Register the MCP `search-records` tool with its config-owned contract.""" description = _build_search_tool_description( + index_ids=index_ids, schema=schema, base_description=server.mcp_settings.tool_search_description, ) diff --git a/tests/unit/test_mcp/test_list_indexes_tool_unit.py b/tests/unit/test_mcp/test_list_indexes_tool_unit.py index 0d56b0833..daaf8903f 100644 --- a/tests/unit/test_mcp/test_list_indexes_tool_unit.py +++ b/tests/unit/test_mcp/test_list_indexes_tool_unit.py @@ -229,3 +229,53 @@ async def test_register_list_indexes_tool_is_read_only_and_callable(): result = await tool["fn"]() assert result == list_indexes(server) + + +def test_list_indexes_reports_upsert_unavailable_when_the_tool_is_disabled(): + """A writable binding is still unwritable if `upsert-records` is not published. + + `upsert_available` is a client-facing claim about what can be called. Deriving + it from read-only state alone would advertise a tool the server withholds, so + the client would only discover the truth by attempting a write. + """ + server = FakeServer([_binding_runtime("knowledge", effective_read_only=False)]) + server.config = MCPConfig.model_validate( + { + "server": { + "redis_url": "redis://localhost:6379", + "builtin_tools": {"upsert-records": "disabled"}, + }, + "indexes": { + "knowledge": { + "redis_name": "docs-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + } + }, + } + ) + + indexes = {entry["id"]: entry for entry in list_indexes(server)["indexes"]} + + assert indexes["knowledge"]["upsert_available"] is False + + +def test_list_indexes_reports_upsert_available_when_the_tool_is_enabled(): + """The control: an explicit config that leaves the built-in on.""" + server = FakeServer([_binding_runtime("knowledge", effective_read_only=False)]) + server.config = MCPConfig.model_validate( + { + "server": {"redis_url": "redis://localhost:6379"}, + "indexes": { + "knowledge": { + "redis_name": "docs-index", + "search": {"type": "fulltext"}, + "runtime": {"text_field_name": "content"}, + } + }, + } + ) + + indexes = {entry["id"]: entry for entry in list_indexes(server)["indexes"]} + + assert indexes["knowledge"]["upsert_available"] is True diff --git a/tests/unit/test_mcp/test_server.py b/tests/unit/test_mcp/test_server.py index ddabde013..8786e0f92 100644 --- a/tests/unit/test_mcp/test_server.py +++ b/tests/unit/test_mcp/test_server.py @@ -62,7 +62,9 @@ def _startup_config(indexes=None): server=SimpleNamespace( redis_url="redis://localhost:6379", # Mirrors MCPServerConfig: every built-in is enabled unless a config - # explicitly disables it. + # explicitly disables it, and the map itself is read when the server + # fingerprints its registered tool surface. + builtin_tools={}, builtin_tool_enabled=lambda _name: True, ), indexes=indexes or {"knowledge": _binding_namespace()}, @@ -412,7 +414,7 @@ async def fake_initialize_vectorizer(self, binding, schema, timeout): registered_schemas = [] - def fake_register_search_tool(server, schema): + def fake_register_search_tool(server, schema, index_ids=None): registered_schemas.append(schema) async def fake_disconnect(self): diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 9f3c8f2f2..fa4871ab8 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -155,7 +155,7 @@ def _register_tools_with(monkeypatch, bindings: dict, *, config=None) -> list[st ) monkeypatch.setattr( "redisvl.mcp.server.register_search_tool", - lambda server, schema: registered.append("search-records"), + lambda server, schema, index_ids=None: registered.append("search-records"), ) monkeypatch.setattr( "redisvl.mcp.server.register_upsert_tool", @@ -165,6 +165,7 @@ def _register_tools_with(monkeypatch, bindings: dict, *, config=None) -> list[st server = RedisVLMCPServer.__new__(RedisVLMCPServer) server._bindings = bindings server._tools_registered = False + server._registered_tool_fingerprint = "" server.tool = object() server.config = config server.mcp_settings = SimpleNamespace(read_only=False) @@ -308,3 +309,92 @@ def test_register_tools_stays_quiet_when_discovery_is_disabled_on_one_index( for record in caplog.records if "cannot discover" in record.message ] + + +def test_register_tools_names_index_ids_when_discovery_is_disabled(monkeypatch): + """A multi-index description must not point at a tool the server withholds.""" + captured: dict = {} + monkeypatch.setattr( + "redisvl.mcp.server.register_list_indexes_tool", lambda server: None + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_search_tool", + lambda server, schema, index_ids=None: captured.update(index_ids=index_ids), + ) + monkeypatch.setattr("redisvl.mcp.server.register_upsert_tool", lambda server: None) + + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._bindings = { + "knowledge": _binding_runtime("knowledge"), + "tickets": _binding_runtime("tickets"), + } + server._tools_registered = False + server._registered_tool_fingerprint = "" + server.tool = object() + server.config = _config_with(builtin_tools={"list-indexes": "disabled"}) + server.mcp_settings = SimpleNamespace(read_only=False) + + server._register_tools() + + # Without discovery these ids are otherwise unlearnable, and `index` is + # required on a multi-index server. + assert captured["index_ids"] == ["knowledge", "tickets"] + + +def test_register_tools_omits_index_ids_when_discovery_is_available(monkeypatch): + """With list-indexes published, the description should defer to it as before.""" + captured: dict = {} + monkeypatch.setattr( + "redisvl.mcp.server.register_list_indexes_tool", lambda server: None + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_search_tool", + lambda server, schema, index_ids=None: captured.update(index_ids=index_ids), + ) + monkeypatch.setattr("redisvl.mcp.server.register_upsert_tool", lambda server: None) + + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._bindings = { + "knowledge": _binding_runtime("knowledge"), + "tickets": _binding_runtime("tickets"), + } + server._tools_registered = False + server._registered_tool_fingerprint = "" + server.tool = object() + server.config = None + server.mcp_settings = SimpleNamespace(read_only=False) + + server._register_tools() + + assert captured["index_ids"] is None + + +def test_register_tools_warns_when_builtin_config_changed_after_registration( + monkeypatch, caplog +): + """Tools register once per process, so an edited config cannot take effect.""" + registered = _register_tools_with( + monkeypatch, + {"knowledge": _binding_runtime("knowledge")}, + config=_config_with(), + ) + assert "upsert-records" in registered + + # Simulate a stop/start that reloaded a config which now disables upsert. + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._bindings = {"knowledge": _binding_runtime("knowledge")} + server.tool = object() + server._tools_registered = True + server._registered_tool_fingerprint = "" + server.config = _config_with(builtin_tools={"upsert-records": "disabled"}) + + with caplog.at_level(logging.WARNING, logger="redisvl.mcp.server"): + server._register_tools() + + # The dangerous direction: an operator disables a tool, restarts, and believes + # it is gone while the old tool set is still what clients see. + assert [ + r.message + for r in caplog.records + if "changed since tools were registered" in r.message + ] From c4d348519be4cf2daa48c841b228926684b6890f Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 13 Aug 2026 10:40:33 +0200 Subject: [PATCH 3/4] fix(mcp): update the integration stub for the new search signature `register_search_tool` gained a keyword-only `index_ids` in the previous commit, but `test_read_only_mode_excludes_upsert_tool` still patched it with a two-argument lambda. Startup raised `TypeError: unexpected keyword argument 'index_ids'` before any read-only assertion ran, so the failure was in the stub rather than the behavior under test. I fixed the two unit-test stubs when changing the signature and did not grep the integration tests, which is the actual mistake -- monkeypatched stubs only fail at call time, so nothing flagged it locally. All five patched call sites are now checked; `register_list_indexes_tool` and `register_upsert_tool` are unchanged, so their stubs still match. Verified by reverting the stub to reproduce the exact CI TypeError, then running the full MCP integration suite: 56 passed, 2 skipped. --- tests/integration/test_mcp/test_upsert_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_mcp/test_upsert_tool.py b/tests/integration/test_mcp/test_upsert_tool.py index a723b30e4..bff7bb916 100644 --- a/tests/integration/test_mcp/test_upsert_tool.py +++ b/tests/integration/test_mcp/test_upsert_tool.py @@ -491,7 +491,7 @@ async def test_read_only_mode_excludes_upsert_tool( ) monkeypatch.setattr( "redisvl.mcp.server.register_search_tool", - lambda server, schema: None, + lambda server, schema, index_ids=None: None, ) def fake_tool(*args: Any, **kwargs: Any): From f323df5d4d9b4639e2f9550c8ab5558c6feb294c Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 13 Aug 2026 14:15:46 +0200 Subject: [PATCH 4/4] fix(mcp): extend the discovery fallback to upsert-records I fixed the unsatisfiable-contract problem for `search-records` and left `upsert-records` with the same defect. Both require an `index` once several bindings exist, so both are equally stranded when `list-indexes` is withheld -- and upsert was arguably worse off, since its description never mentioned discovery at all, leaving a client no route to the ids. `register_upsert_tool` now takes the same `index_ids` and appends them to its description. The condition is computed once in `_register_tools` and passed to both, rather than derived separately at each call, so the two cannot drift apart again the way they just did. The unusable-surface warning was also keyed on `search-records` alone, so a write-only surface with discovery disabled stayed silent. It now reports whichever index-requiring tools are actually published, and names them. Each half has a test that fails when only that half is reverted: one asserting upsert receives the same ids as search, one asserting the warning fires and names `upsert-records` on a surface where search is disabled. Verified against the full MCP integration suite as well this time (56 passed, 2 skipped), since the last signature change broke a stub that only integration exercised. --- docs/concepts/mcp.md | 2 +- redisvl/mcp/server.py | 46 ++++++++----- redisvl/mcp/tools/upsert.py | 17 ++++- .../integration/test_mcp/test_upsert_tool.py | 2 +- tests/unit/test_mcp/test_server.py | 4 +- tests/unit/test_mcp/test_server_unit.py | 69 ++++++++++++++++++- 6 files changed, 114 insertions(+), 26 deletions(-) diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index e8d572619..7d86fe6cd 100644 --- a/docs/concepts/mcp.md +++ b/docs/concepts/mcp.md @@ -124,7 +124,7 @@ Only the three names above are accepted; anything else fails at startup rather t Disabling a built-in adjusts what the rest of the surface advertises, so the published contract never points at something the server withholds: - `list-indexes` reports `upsert_available: false` for every binding when `upsert-records` is disabled, since a writable binding still cannot be written to through a tool that is not published. -- On a multi-index server with `list-indexes` disabled, `search-records` names the available index ids in its own description instead of telling clients to call a discovery tool that does not exist. That server still logs a startup warning, because inlining the ids is a fallback rather than an endorsement of the shape. +- On a multi-index server with `list-indexes` disabled, every tool that requires an `index` — `search-records` and `upsert-records` alike — names the available index ids in its own description instead of deferring to a discovery tool that does not exist. That server still logs a startup warning naming the affected tools, because inlining the ids is a fallback rather than an endorsement of the shape. A server whose tool set ends up unusable — no tools at all, or discovery disabled on a multi-index server — logs a warning at startup. diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 349367481..6613ba4dc 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -319,19 +319,22 @@ def _register_tools(self) -> None: registered: list[str] = [] # Discovery is on by default so clients can enumerate indexes. - if enabled("list-indexes"): + discovery_enabled = enabled("list-indexes") + if discovery_enabled: register_list_indexes_tool(self) registered.append("list-indexes") + + # `index` is required once several bindings exist, and without discovery + # the logical ids cannot be learned any other way -- so every tool that + # requires one has to name them inline instead of deferring to a tool that + # is not published. Computed once so the two cannot drift apart. + unlisted_index_ids = ( + sorted(self._bindings) + if len(self._bindings) > 1 and not discovery_enabled + else None + ) + if enabled("search-records"): - # Without discovery the caller cannot learn the logical ids, so the - # description has to name them rather than pointing at a tool that is - # not published. Only relevant when the schema is ambiguous, which is - # exactly the multi-binding case. - unlisted_index_ids = ( - sorted(self._bindings) - if search_schema is None and "list-indexes" not in registered - else None - ) register_search_tool(self, search_schema, index_ids=unlisted_index_ids) registered.append("search-records") # Expose upsert only when at least one binding is writable. A binding is @@ -341,7 +344,7 @@ def _register_tools(self) -> None: if enabled("upsert-records") and any( not rt.effective_read_only for rt in self._bindings.values() ): - register_upsert_tool(self) + register_upsert_tool(self, index_ids=unlisted_index_ids) registered.append("upsert-records") self._warn_on_unusable_tool_surface(registered) @@ -365,19 +368,26 @@ def _warn_on_unusable_tool_surface(self, registered: list[str]) -> None: ) return - # `search-records`'s multi-index description tells clients to call - # list-indexes first, so disabling discovery leaves them unable to learn - # the logical ids the tool requires. + # Both `search-records` and `upsert-records` require an `index` once + # several bindings exist, so either one is affected by losing discovery -- + # naming them in the descriptions keeps the contract satisfiable, but an + # operator who disabled discovery on a multi-index server probably did not + # intend to. Checking only search would leave a write-only surface silent. + index_requiring = sorted( + {"search-records", "upsert-records"}.intersection(registered) + ) if ( len(self._bindings) > 1 - and "search-records" in registered + and index_requiring and "list-indexes" not in registered ): logger.warning( - "MCP server has %d indexes and exposes search-records, but " - "list-indexes is disabled: clients cannot discover the logical " - "index ids that search-records requires.", + "MCP server has %d indexes and exposes %s, but list-indexes is " + "disabled: clients cannot discover the logical index ids those " + "tools require, so the ids are named inline in each tool " + "description instead.", len(self._bindings), + ", ".join(index_requiring), ) @asynccontextmanager diff --git a/redisvl/mcp/tools/upsert.py b/redisvl/mcp/tools/upsert.py index 4137c9a1a..468ddd89c 100644 --- a/redisvl/mcp/tools/upsert.py +++ b/redisvl/mcp/tools/upsert.py @@ -360,11 +360,24 @@ async def upsert_records( raise map_exception(exc) from exc -def register_upsert_tool(server: Any) -> None: - """Register the MCP upsert tool on a server-like object.""" +def register_upsert_tool(server: Any, *, index_ids: list[str] | None = None) -> None: + """Register the MCP upsert tool on a server-like object. + + ``index_ids`` is supplied only when discovery is unavailable on a multi-index + server. ``index`` is required there, and with ``list-indexes`` withheld the + logical ids cannot be learned any other way, so naming them inline is what + keeps the published contract satisfiable. + """ description = ( server.mcp_settings.tool_upsert_description or DEFAULT_UPSERT_DESCRIPTION ) + if index_ids: + description = ( + description.strip() + " Multiple indexes are configured and discovery " + "is disabled: pass one of these index ids as the `index` argument: " + + ", ".join(index_ids) + + "." + ) async def upsert_records_tool( records: list[dict[str, Any]], diff --git a/tests/integration/test_mcp/test_upsert_tool.py b/tests/integration/test_mcp/test_upsert_tool.py index bff7bb916..ac2b34f09 100644 --- a/tests/integration/test_mcp/test_upsert_tool.py +++ b/tests/integration/test_mcp/test_upsert_tool.py @@ -506,7 +506,7 @@ def decorator(func: Any) -> Any: called: list[bool] = [] - def fake_register_upsert_tool(server: Any) -> None: + def fake_register_upsert_tool(server: Any, index_ids: Any = None) -> None: called.append(server.mcp_settings.read_only) monkeypatch.setattr( diff --git a/tests/unit/test_mcp/test_server.py b/tests/unit/test_mcp/test_server.py index 8786e0f92..27b3437d7 100644 --- a/tests/unit/test_mcp/test_server.py +++ b/tests/unit/test_mcp/test_server.py @@ -429,7 +429,9 @@ async def fake_disconnect(self): monkeypatch.setattr( "redisvl.mcp.server.register_search_tool", fake_register_search_tool ) - monkeypatch.setattr("redisvl.mcp.server.register_upsert_tool", lambda server: None) + monkeypatch.setattr( + "redisvl.mcp.server.register_upsert_tool", lambda server, index_ids=None: None + ) monkeypatch.setattr( "redisvl.mcp.server.register_list_indexes_tool", lambda server: None ) diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index fa4871ab8..1bbfa6513 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -159,7 +159,7 @@ def _register_tools_with(monkeypatch, bindings: dict, *, config=None) -> list[st ) monkeypatch.setattr( "redisvl.mcp.server.register_upsert_tool", - lambda server: registered.append("upsert-records"), + lambda server, index_ids=None: registered.append("upsert-records"), ) server = RedisVLMCPServer.__new__(RedisVLMCPServer) @@ -321,7 +321,9 @@ def test_register_tools_names_index_ids_when_discovery_is_disabled(monkeypatch): "redisvl.mcp.server.register_search_tool", lambda server, schema, index_ids=None: captured.update(index_ids=index_ids), ) - monkeypatch.setattr("redisvl.mcp.server.register_upsert_tool", lambda server: None) + monkeypatch.setattr( + "redisvl.mcp.server.register_upsert_tool", lambda server, index_ids=None: None + ) server = RedisVLMCPServer.__new__(RedisVLMCPServer) server._bindings = { @@ -351,7 +353,9 @@ def test_register_tools_omits_index_ids_when_discovery_is_available(monkeypatch) "redisvl.mcp.server.register_search_tool", lambda server, schema, index_ids=None: captured.update(index_ids=index_ids), ) - monkeypatch.setattr("redisvl.mcp.server.register_upsert_tool", lambda server: None) + monkeypatch.setattr( + "redisvl.mcp.server.register_upsert_tool", lambda server, index_ids=None: None + ) server = RedisVLMCPServer.__new__(RedisVLMCPServer) server._bindings = { @@ -398,3 +402,62 @@ def test_register_tools_warns_when_builtin_config_changed_after_registration( for r in caplog.records if "changed since tools were registered" in r.message ] + + +def test_register_tools_gives_upsert_the_same_index_ids_as_search(monkeypatch): + """Both tools require `index`, so both need the ids when discovery is off.""" + captured: dict = {} + monkeypatch.setattr( + "redisvl.mcp.server.register_list_indexes_tool", lambda server: None + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_search_tool", + lambda server, schema, index_ids=None: captured.update(search=index_ids), + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_upsert_tool", + lambda server, index_ids=None: captured.update(upsert=index_ids), + ) + + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._bindings = { + "knowledge": _binding_runtime("knowledge"), + "tickets": _binding_runtime("tickets"), + } + server._tools_registered = False + server._registered_tool_fingerprint = "" + server.tool = object() + server.config = _config_with(builtin_tools={"list-indexes": "disabled"}) + server.mcp_settings = SimpleNamespace(read_only=False) + + server._register_tools() + + # Writes need the ids exactly as much as reads do. + assert captured["upsert"] == ["knowledge", "tickets"] + assert captured["upsert"] == captured["search"] + + +def test_register_tools_warns_when_discovery_is_disabled_on_a_write_only_surface( + monkeypatch, caplog +): + """A write-only surface loses discovery too, and must not warn silently.""" + with caplog.at_level(logging.WARNING, logger="redisvl.mcp.server"): + registered = _register_tools_with( + monkeypatch, + { + "knowledge": _binding_runtime("knowledge"), + "tickets": _binding_runtime("tickets"), + }, + config=_config_with( + builtin_tools={ + "list-indexes": "disabled", + "search-records": "disabled", + } + ), + ) + + # Only upsert is published, so a check keyed on search-records would miss it. + assert registered == ["upsert-records"] + messages = [r.message for r in caplog.records if "cannot discover" in r.message] + assert messages + assert "upsert-records" in messages[0]