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)