fix(grpc): apply non-blocking review nits flagged across the three review passes

Bundle of low-risk, non-blocking improvements that surfaced during the
deep review iterations. Each item below was explicitly classified as
fix-now in the review summary; broader / design-dependent items were
filed as follow-ups under #4612 and #4613.

* Schema parity (#1): add grpc_service_id to ToolRead in schemas.py
  alongside the existing gateway_id field so API consumers can identify
  gRPC-discovered tools (review S2.4 / TD1).

* db.py cleanup (#2): drop redundant nullable=True from
  Tool.grpc_service_id; Mapped[Optional[str]] already implies it and
  the sibling gateway_id column omits it (review TD2).

* translate_grpc.load_file_descriptors (#3): widen the parameter type
  from List[bytes] to Sequence[bytes] and reject a single bytes object
  passed by mistake. Without the guard Python would silently iterate
  byte-by-byte (review TD3).

* translate_grpc descriptor pool conflict (#4): replace the inaccurate
  'no-op if already added' comment with an explicit TypeError handler.
  protobuf raises TypeError when a file with the same name has
  conflicting content; the existing descriptor stays authoritative
  (review S3.3).

* test_sync_tools_removes_stale_tools (#5): replace the brittle
  string-match assertion ('DELETE' in str(call)) with an explicit
  call_count == 4 (1 select + 3 deletes), which no longer depends on
  the SQLAlchemy Delete object repr (review S2.6).

* translate_grpc close() (#6): convert the f-string log to lazy
  %-style for consistency with the rest of this PR's logging
  (review N2.1).

* grpc_service _sync_tools_from_reflection (#7): document why
  input_schema['properties'] is empty by design — gRPC arg shape is
  validated at the protobuf invocation layer, not the MCP tool-call
  layer; the actual proto types live in the x-grpc-* extensions
  (review N2.2).

* TestInvokeMethodGuards (#8): add 5 edge-case tests covering
  invoke_method paths the previous suite did not exercise:
  - service-not-found  -> GrpcServiceNotFoundError
  - disabled service   -> GrpcServiceError('is disabled')
  - invalid method format (no dot) -> GrpcServiceError
  - _validate_grpc_target spy called with service.target
  - _validate_tls_path spy called for both cert and key paths

525 targeted tests pass (was 520). Lint clean.

Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

Author: jonpspri

mcpgateway/db.py

@@ -3264,7 +3264,7 @@ class Tool(Base):
     # federated_with = relationship("Gateway", secondary=tool_gateway_table, back_populates="federated_tools")
 
     # Federation relationship with a gRPC service
-    grpc_service_id: Mapped[Optional[str]] = mapped_column(ForeignKey("grpc_services.id", ondelete="CASCADE"), nullable=True)
+    grpc_service_id: Mapped[Optional[str]] = mapped_column(ForeignKey("grpc_services.id", ondelete="CASCADE"))
     grpc_service: Mapped[Optional["GrpcService"]] = relationship("GrpcService", back_populates="tools")
 
     # Many-to-many relationship with Servers

mcpgateway/schemas.py

@@ -1518,6 +1518,7 @@ class ToolRead(BaseModelWithConfigDict):
     enabled: bool
     reachable: bool
     gateway_id: Optional[str]
+    grpc_service_id: Optional[str] = Field(None, description="ID of the gRPC service this tool was discovered from")
     execution_count: Optional[int] = Field(None)
     metrics: Optional[ToolMetrics] = Field(None)
     name: str

mcpgateway/services/grpc_service.py

@@ -842,6 +842,9 @@ def _sync_tools_from_reflection(
                 try:
                     _validate_reflected_tool_name(tool_name)
                     description = f"gRPC method {tool_name}"
+                    # ``properties: {}`` is intentional: gRPC argument shape is validated at the
+                    # protobuf invocation layer, not at the MCP tool-call layer. The actual proto
+                    # types are recorded in the x-grpc-* extensions for tooling/inspection.
                     input_schema = {
                         "type": "object",
                         "properties": {},

mcpgateway/translate_grpc.py

@@ -14,7 +14,7 @@
 # Standard
 import asyncio
 from pathlib import Path
-from typing import Any, AsyncGenerator, Dict, List, Optional
+from typing import Any, AsyncGenerator, Dict, List, Optional, Sequence
 
 try:
     # Third-Party
@@ -387,21 +387,26 @@ async def close(self) -> None:
         """Close the gRPC channel."""
         if self._channel:
             self._channel.close()
-            logger.info(f"Closed gRPC connection to {self._target}")
+            logger.info("Closed gRPC connection to %s", self._target)
 
-    def load_file_descriptors(self, file_descriptor_protos: List[bytes]) -> None:
+    def load_file_descriptors(self, file_descriptor_protos: Sequence[bytes]) -> None:
         """Load serialized FileDescriptorProto bytes into the descriptor pool.
 
         This populates the pool with message type definitions so that
         invoke() can serialize/deserialize requests without a reflection
         round-trip.
 
         Args:
-            file_descriptor_protos: List of raw FileDescriptorProto bytes.
+            file_descriptor_protos: Sequence of raw FileDescriptorProto bytes.
+                Passing a single ``bytes`` object directly is rejected because
+                Python would silently iterate it byte-by-byte.
 
         Raises:
-            ValueError: If unable to parse the protobuf descriptor
+            TypeError: If a single bytes object is passed instead of a sequence.
+            ValueError: If unable to parse the protobuf descriptor.
         """
+        if isinstance(file_descriptor_protos, (bytes, bytearray)):
+            raise TypeError("file_descriptor_protos must be a sequence of bytes, not a single bytes object")
         for proto_bytes in file_descriptor_protos:
             fd = FileDescriptorProto()
             try:
@@ -410,8 +415,14 @@ def load_file_descriptors(self, file_descriptor_protos: List[bytes]) -> None:
                 logger.error("Failed to decode protobuf: %s", proto_bytes[:100])
                 raise ValueError("Unable to parse protobuf descriptor") from err
 
-            # NOTE: Adding a descriptor is a no-op if already added
-            self._pool.Add(fd)
+            try:
+                self._pool.Add(fd)
+            except TypeError as conflict_err:
+                # protobuf raises TypeError when a file with the same name is already registered
+                # with conflicting content. The existing descriptor stays authoritative; this is
+                # NOT a true no-op, but treating it as "skip-and-log" matches the prior intent
+                # without masking actual programming errors.
+                logger.debug("Descriptor pool conflict for %s: %s", fd.name, conflict_err)
 
     def get_services(self) -> List[str]:
         """Get list of discovered service names.

tests/unit/mcpgateway/services/test_grpc_service.py

@@ -733,12 +733,10 @@ def test_sync_tools_removes_stale_tools(self, service, mock_db, sample_db_servic
 
         service._sync_tools_from_reflection(mock_db, sample_db_service)
 
-        # Should have executed delete statements for the stale tool
-        # 3 deletes: ToolMetric, server_tool_association, DbTool
-        delete_calls = [call for call in mock_db.execute.call_args_list if "DELETE" in str(call) or "delete" in str(call).lower()]
-        assert len(delete_calls) == 3
-        # At minimum, execute was called for the select + 3 deletes
-        assert mock_db.execute.call_count >= 4  # 1 select + 3 deletes
+        # Stale-tool cleanup fires exactly 3 deletes (ToolMetric, server_tool_association, DbTool)
+        # plus the initial SELECT for existing tools. Asserting on call_count is more robust than
+        # string-matching the SQLAlchemy Delete object repr.
+        assert mock_db.execute.call_count == 4
 
     def test_sync_tools_empty_discovered_services(self, service, mock_db, sample_db_service):
         """Test _sync_tools_from_reflection with empty discovered services and no existing tools."""
@@ -1282,3 +1280,101 @@ def test_sync_isolates_per_tool_failures(self):
 
         # Exactly one DbTool was added (the good one).
         assert db.add.call_count == 1
+
+
+class TestInvokeMethodGuards:
+    """Edge-case coverage for GrpcService.invoke_method security/integrity guards."""
+
+    @pytest.fixture
+    def service(self):
+        return GrpcService()
+
+    @pytest.fixture
+    def mock_db(self):
+        return MagicMock()
+
+    @pytest.mark.asyncio
+    async def test_invoke_method_service_not_found_raises(self, service, mock_db):
+        mock_db.execute.return_value.scalar_one_or_none.return_value = None
+        with pytest.raises(GrpcServiceNotFoundError, match="not found"):
+            await service.invoke_method(mock_db, "missing-id", "svc.M", {})
+
+    @pytest.mark.asyncio
+    async def test_invoke_method_disabled_service_raises(self, service, mock_db):
+        disabled = MagicMock(spec=DbGrpcService)
+        disabled.id = "svc-1"
+        disabled.name = "svc"
+        disabled.enabled = False
+        mock_db.execute.return_value.scalar_one_or_none.return_value = disabled
+        with pytest.raises(GrpcServiceError, match="is disabled"):
+            await service.invoke_method(mock_db, "svc-1", "svc.M", {})
+
+    @pytest.mark.asyncio
+    async def test_invoke_method_invalid_method_format_raises(self, service, mock_db, monkeypatch):
+        enabled = MagicMock(spec=DbGrpcService)
+        enabled.id = "svc-1"
+        enabled.name = "svc"
+        enabled.enabled = True
+        enabled.target = "localhost:50051"
+        enabled.tls_cert_path = None
+        enabled.tls_key_path = None
+        mock_db.execute.return_value.scalar_one_or_none.return_value = enabled
+        # Bypass real network/TLS validation; the GuardCheck runs after the format check.
+        monkeypatch.setattr("mcpgateway.services.grpc_service._validate_grpc_target", lambda _t: None)
+        with pytest.raises(GrpcServiceError, match="Invalid method name"):
+            await service.invoke_method(mock_db, "svc-1", "NoDotMethod", {})
+
+    @pytest.mark.asyncio
+    async def test_invoke_method_calls_target_validator(self, service, mock_db, monkeypatch):
+        enabled = MagicMock(spec=DbGrpcService)
+        enabled.id = "svc-1"
+        enabled.name = "svc"
+        enabled.enabled = True
+        enabled.target = "host.example:50051"
+        enabled.tls_cert_path = None
+        enabled.tls_key_path = None
+        enabled.discovered_services = {}
+        enabled.tls_enabled = False
+        enabled.grpc_metadata = {}
+        mock_db.execute.return_value.scalar_one_or_none.return_value = enabled
+
+        # The spy raises a sentinel error so the test asserts the spy was called and exits the
+        # invoke flow before reaching the (network-dependent) GrpcEndpoint construction.
+        sentinel = GrpcServiceError("spy-aborted")
+        spy = MagicMock(side_effect=sentinel)
+        monkeypatch.setattr("mcpgateway.services.grpc_service._validate_grpc_target", spy)
+
+        with pytest.raises(GrpcServiceError, match="spy-aborted"):
+            await service.invoke_method(mock_db, "svc-1", "svc.M", {})
+        spy.assert_called_once_with("host.example:50051")
+
+    @pytest.mark.asyncio
+    async def test_invoke_method_calls_tls_validator_when_paths_set(self, service, mock_db, monkeypatch):
+        enabled = MagicMock(spec=DbGrpcService)
+        enabled.id = "svc-1"
+        enabled.name = "svc"
+        enabled.enabled = True
+        enabled.target = "host.example:50051"
+        enabled.tls_cert_path = "/tls/cert.pem"
+        enabled.tls_key_path = "/tls/key.pem"
+        enabled.discovered_services = {}
+        enabled.tls_enabled = True
+        enabled.grpc_metadata = {}
+        mock_db.execute.return_value.scalar_one_or_none.return_value = enabled
+
+        monkeypatch.setattr("mcpgateway.services.grpc_service._validate_grpc_target", lambda _t: None)
+        # The TLS spy raises after the second call so we both assert it was invoked AND short-circuit
+        # before GrpcEndpoint construction tries to open a real channel.
+        tls_calls: list = []
+
+        def tls_spy(path, label="TLS path"):
+            tls_calls.append((path, label))
+            if len(tls_calls) == 2:
+                raise GrpcServiceError("spy-aborted")
+            return None
+
+        monkeypatch.setattr("mcpgateway.services.grpc_service._validate_tls_path", tls_spy)
+        with pytest.raises(GrpcServiceError, match="spy-aborted"):
+            await service.invoke_method(mock_db, "svc-1", "svc.M", {})
+        labels = {label for _path, label in tls_calls}
+        assert labels == {"TLS cert path", "TLS key path"}