From 2270ccd225f7b2faffa464cdb848c5a63a2e492a Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Tue, 16 Jun 2026 11:11:49 +0200 Subject: [PATCH] feat(mcp): add index routing and per-index write policy to upsert-records (RAAE-1607) Add an optional `index` argument to the upsert-records tool so a multi-binding MCP server can target a specific logical index for writes. As with search, the argument is optional on single-binding servers and required when multiple bindings exist; resolution flows through the shared resolve_binding routing so an omitted index on a multi-binding server and unknown ids both surface as invalid_request. The resolved logical id is echoed back as the `index` field in the response, and the selected binding's embedding, runtime limits, and schema validation are used throughout. Write availability is now enforced at two levels. The upsert tool is registered only when at least one binding is writable, so an all-read-only server (whether from global read-only mode or every binding's own read_only policy) does not advertise the tool at all. When the tool is registered, a per-call check rejects writes to any individual read-only binding with invalid_request before any embedding or backend write occurs, so a writable server can still protect specific indexes. - Expose `index` on the FastMCP wrapper param list. - Refine the registration gate from "global read-only off" to "any binding writable" using effective_read_only, which folds in both global and per-index read-only. - Add unit + integration coverage for routing, omitted-index rejection, unknown ids, read-only rejection, the registration gate, and single-binding backward compatibility. Co-Authored-By: Claude Opus 4.8 (1M context) --- redisvl/mcp/server.py | 6 +- redisvl/mcp/tools/upsert.py | 27 +++- .../integration/test_mcp/test_upsert_tool.py | 124 ++++++++++++++++++ tests/unit/test_mcp/test_server_unit.py | 61 ++++++++- tests/unit/test_mcp/test_upsert_tool_unit.py | 70 +++++++++- 5 files changed, 279 insertions(+), 9 deletions(-) diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index e7c0481d..495adead 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -252,7 +252,11 @@ def _register_tools(self) -> None: # Discovery is always available so clients can enumerate indexes. register_list_indexes_tool(self) register_search_tool(self, search_schema) - if not self.mcp_settings.read_only: + # 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()): register_upsert_tool(self) self._tools_registered = True diff --git a/redisvl/mcp/tools/upsert.py b/redisvl/mcp/tools/upsert.py index eb72e08d..e7bdf797 100644 --- a/redisvl/mcp/tools/upsert.py +++ b/redisvl/mcp/tools/upsert.py @@ -249,13 +249,27 @@ async def upsert_records( server: Any, *, records: list[dict[str, Any]], + index: str | None = None, id_field: str | None = None, skip_embedding_if_present: bool | None = None, ) -> dict[str, Any]: - """Execute `upsert-records` against the selected Redis index binding.""" + """Execute `upsert-records` against the selected Redis index binding. + + ``index`` names the logical binding to write to. It is optional when exactly + one binding is configured and required when multiple exist. Writes to a + read-only binding (whether from global read-only mode or the binding's own + ``read_only`` policy) are rejected with ``invalid_request``. The resolved + logical id is echoed back in the response. + """ try: - rt = server.resolve_binding(None) - index = rt.index + rt = server.resolve_binding(index) + if rt.effective_read_only: + raise RedisVLMCPError( + f"index '{rt.binding_id}' is read-only", + code=MCPErrorCode.INVALID_REQUEST, + retryable=False, + ) + index_obj = rt.index runtime = rt.binding.runtime effective_skip_embedding = _validate_request( runtime=runtime, @@ -269,7 +283,7 @@ async def upsert_records( for record in prepared_records: _validate_record( record, - index=index, + index=index_obj, vector_field_name=runtime.vector_field_name, ) if rt.binding.supports_server_side_embedding: @@ -326,7 +340,7 @@ async def upsert_records( try: keys = await server.run_guarded( "upsert-records", - index.load(loadable_records, id_field=id_field), + index_obj.load(loadable_records, id_field=id_field), timeout_seconds=runtime.request_timeout_seconds, ) except Exception as exc: @@ -335,6 +349,7 @@ async def upsert_records( raise mapped from exc return { + "index": rt.binding_id, "status": "success", "keys_upserted": len(keys), "keys": keys, @@ -353,6 +368,7 @@ def register_upsert_tool(server: Any) -> None: async def upsert_records_tool( records: list[dict[str, Any]], + index: str | None = None, id_field: str | None = None, skip_embedding_if_present: bool | None = None, ): @@ -362,6 +378,7 @@ async def upsert_records_tool( return await upsert_records( server, records=records, + index=index, id_field=id_field, skip_embedding_if_present=skip_embedding_if_present, ) diff --git a/tests/integration/test_mcp/test_upsert_tool.py b/tests/integration/test_mcp/test_upsert_tool.py index 6711790b..f181a158 100644 --- a/tests/integration/test_mcp/test_upsert_tool.py +++ b/tests/integration/test_mcp/test_upsert_tool.py @@ -356,6 +356,130 @@ async def fail_load(*args: Any, **kwargs: Any) -> Any: assert called is False +@pytest.fixture +async def multi_index_upsert_server( + monkeypatch, upsertable_index, fulltext_only_upsert_index, tmp_path, redis_url +): + monkeypatch.setattr( + "redisvl.mcp.server.resolve_vectorizer_class", + lambda class_name: RecordingVectorizer, + ) + + config = { + "server": {"redis_url": redis_url}, + "indexes": { + "knowledge": { + "redis_name": upsertable_index.schema.index.name, + "search": {"type": "vector"}, + "vectorizer": { + "class": "RecordingVectorizer", + "model": "fake-model", + "dims": 3, + }, + "runtime": { + "text_field_name": "content", + "vector_field_name": "embedding", + "default_embed_text_field": "content", + "default_limit": 2, + "max_limit": 5, + "max_upsert_records": 64, + "skip_embedding_if_present": True, + }, + }, + "tickets": { + "redis_name": fulltext_only_upsert_index.schema.index.name, + "read_only": True, + "search": {"type": "fulltext", "params": {"stopwords": None}}, + "runtime": { + "text_field_name": "content", + "vector_field_name": None, + "default_embed_text_field": None, + "default_limit": 2, + "max_limit": 5, + "max_upsert_records": 64, + }, + }, + }, + } + config_path = tmp_path / "multi-index-upsert.yaml" + config_path.write_text(yaml.safe_dump(config), encoding="utf-8") + + server = RedisVLMCPServer(MCPSettings(config=str(config_path))) + await server.startup() + try: + yield server + finally: + await server.shutdown() + + +@pytest.mark.asyncio +async def test_upsert_records_routes_to_named_writable_binding( + multi_index_upsert_server, +): + response = await upsert_records( + multi_index_upsert_server, + index="knowledge", + records=[{"content": "routed document", "category": "science", "rating": 5}], + ) + + assert response["index"] == "knowledge" + assert response["status"] == "success" + assert response["keys_upserted"] == 1 + + +@pytest.mark.asyncio +async def test_upsert_records_requires_index_when_multiple_bindings( + multi_index_upsert_server, +): + with pytest.raises(RedisVLMCPError) as exc_info: + await upsert_records( + multi_index_upsert_server, + records=[{"content": "no index", "category": "science"}], + ) + + assert exc_info.value.code == MCPErrorCode.INVALID_REQUEST + + +@pytest.mark.asyncio +async def test_upsert_records_rejects_unknown_index_on_multi_binding( + multi_index_upsert_server, +): + with pytest.raises(RedisVLMCPError) as exc_info: + await upsert_records( + multi_index_upsert_server, + index="missing", + records=[{"content": "doc", "category": "science"}], + ) + + assert exc_info.value.code == MCPErrorCode.INVALID_REQUEST + + +@pytest.mark.asyncio +async def test_upsert_records_rejects_writes_to_read_only_binding( + multi_index_upsert_server, +): + with pytest.raises(RedisVLMCPError, match="read-only") as exc_info: + await upsert_records( + multi_index_upsert_server, + index="tickets", + records=[{"content": "doc", "category": "operations"}], + ) + + assert exc_info.value.code == MCPErrorCode.INVALID_REQUEST + + +@pytest.mark.asyncio +async def test_upsert_records_single_binding_echoes_index_when_omitted(started_server): + server = await started_server() + + response = await upsert_records( + server, + records=[{"content": "solo document", "category": "science", "rating": 5}], + ) + + assert response["index"] == "knowledge" + + @pytest.mark.asyncio async def test_read_only_mode_excludes_upsert_tool( monkeypatch, upsertable_index, mcp_config_path diff --git a/tests/unit/test_mcp/test_server_unit.py b/tests/unit/test_mcp/test_server_unit.py index 9e8af87a..45183791 100644 --- a/tests/unit/test_mcp/test_server_unit.py +++ b/tests/unit/test_mcp/test_server_unit.py @@ -53,7 +53,9 @@ async def test_probe_native_hybrid_search_false_for_old_redis_py(monkeypatch): assert client.info_calls == 0 -def _binding_runtime(binding_id: str) -> BindingRuntime: +def _binding_runtime( + binding_id: str, *, effective_read_only: bool = False +) -> BindingRuntime: return BindingRuntime( binding_id=binding_id, binding=SimpleNamespace(), @@ -61,7 +63,7 @@ def _binding_runtime(binding_id: str) -> BindingRuntime: schema=SimpleNamespace(), vectorizer=None, supports_native_hybrid_search=False, - effective_read_only=False, + effective_read_only=effective_read_only, ) @@ -110,3 +112,58 @@ def test_resolve_binding_rejects_unknown_index(): assert excinfo.value.code == MCPErrorCode.INVALID_REQUEST assert "missing" in str(excinfo.value) + + +def _register_tools_with(monkeypatch, bindings: dict) -> list[str]: + """Run _register_tools against the given bindings, returning registered names.""" + registered: list[str] = [] + monkeypatch.setattr( + "redisvl.mcp.server.register_list_indexes_tool", + lambda server: registered.append("list-indexes"), + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_search_tool", + lambda server, schema: registered.append("search-records"), + ) + monkeypatch.setattr( + "redisvl.mcp.server.register_upsert_tool", + lambda server: registered.append("upsert-records"), + ) + + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._bindings = bindings + server._tools_registered = False + server.tool = object() + server.mcp_settings = SimpleNamespace(read_only=False) + + server._register_tools() + return registered + + +def test_register_tools_exposes_upsert_when_a_binding_is_writable(monkeypatch): + registered = _register_tools_with( + monkeypatch, + { + "knowledge": _binding_runtime("knowledge", effective_read_only=False), + "tickets": _binding_runtime("tickets", effective_read_only=True), + }, + ) + + assert "upsert-records" in registered + assert "list-indexes" in registered + assert "search-records" in registered + + +def test_register_tools_hides_upsert_when_every_binding_is_read_only(monkeypatch): + registered = _register_tools_with( + monkeypatch, + { + "knowledge": _binding_runtime("knowledge", effective_read_only=True), + "tickets": _binding_runtime("tickets", effective_read_only=True), + }, + ) + + assert "upsert-records" not in registered + # Read paths stay available even when writes are globally disabled. + assert "list-indexes" in registered + assert "search-records" in registered diff --git a/tests/unit/test_mcp/test_upsert_tool_unit.py b/tests/unit/test_mcp/test_upsert_tool_unit.py index d47046ce..d34e0792 100644 --- a/tests/unit/test_mcp/test_upsert_tool_unit.py +++ b/tests/unit/test_mcp/test_upsert_tool_unit.py @@ -150,6 +150,7 @@ def __init__( max_upsert_records: int = 5, skip_embedding_if_present: bool = True, vectorizer: FakeVectorizer | None = None, + effective_read_only: bool = False, ): self.config = _config( storage_type, @@ -164,8 +165,17 @@ def __init__( self.index = FakeIndex(storage_type, include_vector_field=include_vector_field) self.vectorizer = vectorizer or FakeVectorizer() if include_vectorizer else None self.registered_tools = [] + self.effective_read_only = effective_read_only + self.resolved_index_ids: list[str | None] = [] def resolve_binding(self, index_id=None): + self.resolved_index_ids.append(index_id) + if index_id is not None and index_id != "knowledge": + raise RedisVLMCPError( + f"Unknown index '{index_id}'; available: knowledge", + code=MCPErrorCode.INVALID_REQUEST, + retryable=False, + ) return BindingRuntime( binding_id="knowledge", binding=self.config.indexes["knowledge"], @@ -173,7 +183,7 @@ def resolve_binding(self, index_id=None): schema=self.index.schema, vectorizer=self.vectorizer, supports_native_hybrid_search=False, - effective_read_only=False, + effective_read_only=self.effective_read_only, ) async def get_index(self): @@ -218,6 +228,7 @@ async def test_upsert_records_generates_missing_vectors_and_serializes_hash_vect ) assert response == { + "index": "knowledge", "status": "success", "keys_upserted": 2, "keys": ["doc:alpha", "doc:beta"], @@ -463,6 +474,63 @@ async def test_upsert_records_surfaces_partial_write_possible_on_backend_failure assert isinstance(exc_info.value.__cause__, RedisError) +@pytest.mark.asyncio +async def test_upsert_records_defaults_to_sole_binding_when_index_omitted(): + server = FakeServer() + + response = await upsert_records(server, records=[{"content": "alpha doc"}]) + + assert server.resolved_index_ids == [None] + assert response["index"] == "knowledge" + + +@pytest.mark.asyncio +async def test_upsert_records_routes_to_named_index(): + server = FakeServer() + + response = await upsert_records( + server, records=[{"content": "alpha doc"}], index="knowledge" + ) + + assert server.resolved_index_ids == ["knowledge"] + assert response["index"] == "knowledge" + + +@pytest.mark.asyncio +async def test_upsert_records_rejects_unknown_index(): + server = FakeServer() + + with pytest.raises(RedisVLMCPError) as exc_info: + await upsert_records( + server, records=[{"content": "alpha doc"}], index="missing" + ) + + assert exc_info.value.code == MCPErrorCode.INVALID_REQUEST + assert server.resolved_index_ids == ["missing"] + assert server.index.load_calls == [] + + +@pytest.mark.asyncio +async def test_upsert_records_rejects_writes_to_read_only_binding(): + server = FakeServer(effective_read_only=True) + + with pytest.raises(RedisVLMCPError, match="read-only") as exc_info: + await upsert_records(server, records=[{"content": "alpha doc"}]) + + assert exc_info.value.code == MCPErrorCode.INVALID_REQUEST + # Write policy is enforced before any embedding or backend write. + assert server.index.load_calls == [] + assert server.vectorizer.aembed_many_calls == [] + + +def test_register_upsert_tool_wrapper_exposes_index_param(): + server = FakeServer() + register_upsert_tool(server) + + annotations = server.registered_tools[0]["fn"].__annotations__ + assert "index" in annotations + + def test_register_upsert_tool_uses_default_and_override_descriptions(): default_server = FakeServer() register_upsert_tool(default_server)