fix: Address PR review feedback

- Add warning logging when resolve_operation falls back to full tool name
- Document that resolve_operation is for tracing/status, not routing
- Add proper types to Tool class (Callable, ToolDefinition) with ConfigDict
- Remove unnecessary pass statement in prometheus provider
- Clarify _invoke closure default arg captures method to avoid late binding
- Add explanatory comments for AttributeError handlers probing response formats
- Simplify getattr comment in tools() method

Author: abrookins

redis_sre_agent/agent/langgraph_agent.py

@@ -1855,19 +1855,22 @@ async def _retry_with_backoff(
             try:
                 return await func()
             except Exception as e:
-                # Enrich logging with OpenAI/HTTP error details when available
+                # Enrich logging with OpenAI/HTTP error details when available.
+                # Different exception types expose status/body via different attributes,
+                # so we probe multiple patterns and ignore AttributeError when not found.
                 try:
                     status = None
                     body = None
                     try:
                         status = e.status_code  # type: ignore[attr-defined]
                     except AttributeError:
+                        # Try .response.status_code pattern (e.g., httpx exceptions)
                         try:
                             resp = e.response  # type: ignore[attr-defined]
                             if resp:
                                 status = resp.status_code
                         except AttributeError:
-                            pass
+                            pass  # No status code available
                     try:
                         raw_body = e.body  # type: ignore[attr-defined]
                         if isinstance(raw_body, (str, bytes)):
@@ -1877,6 +1880,7 @@ async def _retry_with_backoff(
                                 else raw_body
                             )
                     except AttributeError:
+                        # Try .response.text or .response.content pattern
                         try:
                             resp = e.response  # type: ignore[attr-defined]
                             if resp is not None:
@@ -1886,11 +1890,11 @@ async def _retry_with_backoff(
                                     try:
                                         body = resp.content
                                     except AttributeError:
-                                        pass
+                                        pass  # No body available
                                 if isinstance(body, (bytes, bytearray)):
                                     body = body.decode("utf-8", errors="ignore")
                         except AttributeError:
-                            pass
+                            pass  # No response object available
                     if status or body:
                         snippet = (body or "")[:2000]
                         logger.error(

redis_sre_agent/observability/llm_metrics.py

@@ -42,18 +42,19 @@
 
 
 def _get_model_name(llm: Any) -> str:
+    """Extract model name from LLM object, trying common attribute patterns."""
     try:
         model = llm.model  # type: ignore[attr-defined]
         if model:
             return model
     except AttributeError:
-        pass
+        pass  # LLM may not have .model attribute
     try:
         model = llm._model  # type: ignore[attr-defined]
         if model:
             return model
     except AttributeError:
-        pass
+        pass  # LLM may not have ._model attribute
     return "unknown"
 
 
@@ -72,7 +73,7 @@ def _extract_usage_from_response(resp: Any) -> Dict[str, Optional[int]]:
             completion = usage_md.get("output_tokens") or usage_md.get("completion_tokens")
             total = usage_md.get("total_tokens")
     except AttributeError:
-        pass
+        pass  # Response may not have .usage_metadata attribute
 
     # Some wrappers place the raw OpenAI usage in response_metadata
     if prompt is None or completion is None:
@@ -88,7 +89,7 @@ def _extract_usage_from_response(resp: Any) -> Dict[str, Optional[int]]:
                     )
                     total = total or usage.get("total_tokens")
         except AttributeError:
-            pass
+            pass  # Response may not have .response_metadata attribute
 
     # Many providers only give total
     if total is None and prompt is not None and completion is not None:

redis_sre_agent/tools/metrics/prometheus/provider.py

@@ -226,8 +226,6 @@ async def _wait_for_targets(self, timeout_seconds: float = 10.0) -> None:
         if last_err:
             logger.debug(f"_wait_for_targets encountered: {last_err}")
 
-        pass
-
     async def _http_get_json(
         self, path: str, params: Optional[Dict[str, Any]] = None
     ) -> Dict[str, Any]:

redis_sre_agent/tools/models.py

@@ -1,7 +1,10 @@
 from enum import Enum
-from typing import Any, Dict, Optional
+from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, Optional
 
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, ConfigDict, Field
+
+if TYPE_CHECKING:
+    from redis_sre_agent.tools.models import ToolDefinition
 
 
 class ToolCapability(Enum):
@@ -21,8 +24,7 @@ class ToolMetadata(BaseModel):
     """Metadata about a concrete tool implementation.
 
     This is attached to the :class:`Tool` wrapper alongside the
-    :class:`~redis_sre_agent.tools.tool_definition.ToolDefinition` schema
-    and the callable used to execute the tool.
+    :class:`ToolDefinition` schema and the callable used to execute the tool.
     """
 
     name: str
@@ -37,15 +39,16 @@ class Tool(BaseModel):
 
     Attributes:
         metadata: :class:`ToolMetadata` describing the tool for routing.
-        schema: The :class:`ToolDefinition` shown to the LLM (stored as ``Any``
-            here to avoid import cycles).
+        schema: The :class:`ToolDefinition` shown to the LLM.
         invoke: Async callable taking a single ``Dict[str, Any]`` of arguments
             and returning the tool result.
     """
 
+    model_config = ConfigDict(arbitrary_types_allowed=True)
+
     metadata: ToolMetadata
-    schema: Any
-    invoke: Any
+    schema: "ToolDefinition"
+    invoke: Callable[[Dict[str, Any]], Awaitable[Any]]
 
 
 class SystemHost(BaseModel):

redis_sre_agent/tools/protocols.py

@@ -224,7 +224,14 @@ def resolve_operation(self, tool_name: str, args: Dict[str, Any]) -> Optional[st
 
         Providers can override this for more complex mappings (for example,
         when multiple tool names share a single implementation).
+
+        Note: This method is used for OTel tracing and status updates, not for
+        routing. The ToolManager validates tool existence via the routing table
+        before this is called.
         """
+        import logging
+
+        _logger = logging.getLogger(__name__)
 
         try:
             # Preferred fast-path: exact prefix match based on provider name
@@ -241,8 +248,14 @@ def resolve_operation(self, tool_name: str, args: Dict[str, Any]) -> Optional[st
                 return "_".join(parts[2:])
 
             # Last resort: treat the whole name as the operation.
+            # Log a warning since this may indicate a misconfigured tool name.
+            _logger.warning(
+                f"resolve_operation falling back to full tool name '{tool_name}' as operation. "
+                f"Expected format: {self.provider_name}_<hash>_<operation>"
+            )
             return tool_name
-        except Exception:
+        except Exception as e:
+            _logger.warning(f"resolve_operation failed for '{tool_name}': {e}")
             return None
 
     @property
@@ -363,10 +376,8 @@ def tools(self) -> List["Tool"]:
                 requires_instance=requires_instance,
             )
 
-            # Prefer a direct provider method derived from the tool name.
+            # Derive operation name and look up the corresponding method.
             op_name = self.resolve_operation(schema.name, {}) or ""
-            # Check if method exists on the class
-            # Use getattr for legitimate metaprogramming - dynamically binding methods
             method = getattr(self, op_name, None) if op_name else None
 
             if not callable(method):
@@ -376,8 +387,8 @@ def tools(self) -> List["Tool"]:
                     "or override tools() to provide a custom implementation."
                 )
 
+            # Capture method via default arg to avoid late binding in the closure.
             async def _invoke(args: Dict[str, Any], _method=method) -> Any:
-                """Invoke the bound provider method with keyword args."""
                 return await _method(**(args or {}))
 
             tools.append(Tool(metadata=meta, schema=schema, invoke=_invoke))