refactor(tools): Refactor tool provider system to prepare for MCP support #23
Conversation
- Introduce Tool model combining schema, metadata, and invoke callable - Move ToolCapability, ToolMetadata, and ToolDefinition to tools/models.py - Add automatic wiring in ToolProvider.tools() that binds tool names to methods - Update all providers to use create_tool_schemas() + method pattern - Standardize provider_name as @Property across all providers - Rename redis_cli provider to redis_command - Update documentation and tests to match new patterns - Fix pre-existing linting issues (N806, N814, E402)
There was a problem hiding this comment.
Pull request overview
This PR refactors the tool provider system to prepare for MCP (Model Context Protocol) support by introducing a unified Tool model that combines schema, metadata, and execution callable. The refactoring standardizes patterns across all tool providers and includes cleanup of pre-existing linting issues.
Key changes:
- Introduced
Toolmodel combiningToolMetadata,ToolDefinitionschema, andinvokecallable - Moved
ToolCapability,ToolMetadata, andToolDefinitionfromtool_definition.pyandprotocols.pyto newmodels.py - Implemented automatic method wiring in
ToolProvider.tools()that binds tool names directly to provider methods, eliminating the need for manualresolve_tool_call()implementations - Standardized
provider_nameas@propertyacross all providers - Renamed
redis_cliprovider toredis_commandfor clarity - Replaced defensive
getattr()calls with direct attribute access throughout the codebase
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
redis_sre_agent/tools/models.py |
New file consolidating Tool, ToolCapability, ToolMetadata, ToolDefinition, and SystemHost models |
redis_sre_agent/tools/protocols.py |
Refactored ToolProvider base class with automatic method wiring via tools() and removed weakref pattern for _manager |
redis_sre_agent/tools/tool_definition.py |
File deleted; content moved to models.py |
redis_sre_agent/tools/manager.py |
Updated to work with Tool objects instead of ToolDefinition schemas; added _tool_by_name lookup |
redis_sre_agent/tools/*/provider.py |
All providers updated to remove resolve_tool_call(), add capability to schemas, and use @Property for provider_name |
tests/unit/tools/ |
Tests updated to use tool.invoke() instead of resolve_tool_call() and removed tests for unknown operation errors |
tests/integration/tools/ |
Integration tests updated to call tool.invoke() and verify capability metadata |
redis_sre_agent/core/ |
Multiple files updated replacing getattr() with direct attribute access for better type safety |
redis_sre_agent/agent/ |
Agent code updated to use new Tool model and build_adapters_for_tooldefs signature |
pyproject.toml |
Migrated dev-dependencies to PEP 735 dependency-groups format |
docs/how-to/tool-providers.md |
Documentation updated showing new provider pattern without resolve_tool_call() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
redis_sre_agent/tools/models.py
Outdated
| class Tool(BaseModel): | ||
| """Concrete tool object combining schema, metadata, and executor. | ||
| 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). | ||
| invoke: Async callable taking a single ``Dict[str, Any]`` of arguments | ||
| and returning the tool result. | ||
| """ | ||
|
|
||
| metadata: ToolMetadata | ||
| schema: Any | ||
| invoke: Any |
There was a problem hiding this comment.
The Tool class uses Any for both schema and invoke fields without proper type hints or validation. This can lead to runtime errors if incorrect types are passed. Consider using proper typing with Protocol or more specific types:
from typing import Awaitable, Callable, Union
class Tool(BaseModel):
"""Concrete tool object combining schema, metadata, and executor."""
metadata: ToolMetadata
schema: ToolDefinition # More specific type instead of Any
invoke: Callable[[Dict[str, Any]], Awaitable[Any]] # Proper callable signature
class Config:
arbitrary_types_allowed = True # Allow non-Pydantic types like callables| async def _invoke(args: Dict[str, Any], _method=method) -> Any: | ||
| """Invoke the bound provider method with keyword args.""" | ||
| return await _method(**(args or {})) |
There was a problem hiding this comment.
[nitpick] The closure in _invoke captures _method with a default argument, but the loop variable method changes on each iteration. This could lead to all tools invoking the last method in the loop. The current implementation with _method=method is correct, but the comment should clarify this is intentional to avoid late binding issues.
redis_sre_agent/tools/protocols.py
Outdated
| # Use getattr for legitimate metaprogramming - dynamically binding methods | ||
| method = getattr(self, op_name, None) if op_name else None |
There was a problem hiding this comment.
[nitpick] Using getattr for legitimate metaprogramming is fine, but the comment "Use getattr for legitimate metaprogramming - dynamically binding methods" should be removed as it's defensive and unnecessary. The usage here is straightforward method lookup by name.
| # Last resort: treat the whole name as the operation. | ||
| return tool_name |
There was a problem hiding this comment.
The resolve_operation method has a fallback that treats the whole tool name as the operation (return tool_name). This could mask errors where tool names don't follow the expected pattern. Consider logging a warning or raising an error instead to catch misconfigurations early.
| # Back-reference to the manager (set by ToolManager on load) | ||
| _manager: Optional["ToolManager"] = None |
There was a problem hiding this comment.
The code removes the _manager weak reference pattern and replaces it with a direct reference. This could potentially create reference cycles that prevent garbage collection. If ToolProvider instances hold a reference to ToolManager and ToolManager holds references to providers in _providers and _routing_table, this creates a cycle. The weak reference pattern was likely intentional to avoid this.
Consider keeping the weak reference pattern or documenting why it's safe to remove.
| completion or usage.get("completion_tokens") or usage.get("output_tokens") | ||
| ) | ||
| total = total or usage.get("total_tokens") | ||
| except AttributeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except AttributeError: | |
| except AttributeError: | |
| # It's expected that some response objects may not have response_metadata. |
| resp = e.response # type: ignore[attr-defined] | ||
| if resp: | ||
| status = resp.status_code | ||
| except AttributeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except AttributeError: | ||
| try: | ||
| body = resp.content | ||
| except AttributeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except AttributeError: | |
| except AttributeError: | |
| # Safe to ignore: response object does not have 'content' attribute |
| pass | ||
| if isinstance(body, (bytes, bytearray)): | ||
| body = body.decode("utf-8", errors="ignore") | ||
| except AttributeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except AttributeError: | |
| except AttributeError: | |
| # Attribute missing on response object; safe to ignore |
| if last_err: | ||
| logger.debug(f"_wait_for_targets encountered: {last_err}") | ||
|
|
||
| pass |
There was a problem hiding this comment.
Unnecessary 'pass' statement.
- 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
redis_sre_agent/tools/models.py:7
- The module 'redis_sre_agent.tools.models' imports itself.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not op: | ||
| return None | ||
| method = getattr(self, op, None) | ||
| method = self.__dict__.get(op) or type(self).__dict__.get(op) |
There was a problem hiding this comment.
Using __dict__.get() and type(self).__dict__.get() for attribute lookup is unusual and error-prone. The first call self.__dict__.get(op) will only find instance attributes, missing class-level methods. The second call type(self).__dict__.get(op) will miss inherited methods from parent classes. This should use getattr(self, op, None) to properly traverse the MRO (Method Resolution Order) and find both instance and class attributes correctly.
redis_sre_agent/tools/protocols.py
Outdated
| method = self.__dict__.get(op) or type(self).__dict__.get(op) | ||
| if not method: | ||
| return None | ||
| template = getattr(method, "_status_update_template", None) | ||
| template = method._status_update_template if method else None |
There was a problem hiding this comment.
Accessing _status_update_template with a conditional expression on line 412 is inconsistent with line 409. If method is None (from line 410), this will raise an AttributeError. The access pattern should be consistent: either use getattr or handle None properly.
redis_sre_agent/tools/manager.py
Outdated
| module_path, class_name = provider_path.rsplit(".", 1) | ||
| module = __import__(module_path, fromlist=[class_name]) | ||
| cls._provider_class_cache[provider_path] = getattr(module, class_name) | ||
| cls._provider_class_cache[provider_path] = module.__dict__[class_name] |
There was a problem hiding this comment.
Using module.__dict__[class_name] instead of getattr(module, class_name) removes the ability to handle missing attributes gracefully. If the class doesn't exist in the module, this will raise a KeyError instead of the expected AttributeError, potentially breaking error handling code that catches AttributeError.
redis_sre_agent/cli/main.py
Outdated
| module_path, attr = target.split(":", 1) | ||
| mod = importlib.import_module(module_path) | ||
| return getattr(mod, attr) | ||
| return mod.__dict__[attr] |
There was a problem hiding this comment.
Using mod.__dict__[attr] (line 39) instead of getattr(mod, attr) will raise KeyError instead of AttributeError when the attribute doesn't exist. This breaks the expected exception contract and could cause issues with error handling that specifically catches AttributeError.
| return mod.__dict__[attr] | |
| return getattr(mod, attr) |
| ic = self.instance_config | ||
| if ic and ic.prefer_streams: |
There was a problem hiding this comment.
Direct attribute access without checking if instance_config exists. While the try/except block provides some protection, line 382 assumes ic (which is self.instance_config) is not None when checking ic.prefer_streams. If instance_config_model is set but _load_instance_extension_config() returned None, this could cause an AttributeError. Should check ic is not None before accessing its attributes.
| "created_at", | ||
| "updated_at", | ||
| ]: | ||
| row[k] = res.__dict__.get(k) |
There was a problem hiding this comment.
Same issue: accessing res.__dict__.get(k) (line 418) assumes res has a __dict__ attribute. Should use getattr(res, k, None) for safer attribute access that works with all Python objects.
| row[k] = res.__dict__.get(k) | |
| row[k] = getattr(res, k, None) |
Simplify the property to return bool instead of Optional[bool]. Base class returns whether redis_instance was provided; subclasses override to return True or False explicitly.
- Rename Tool.schema to Tool.definition to avoid shadowing BaseModel attribute - Use getattr for llm.model and llm.temperature to handle ChatOpenAI which uses model_name
Use module-level tracer instead of defensive inline imports
Use module-level tracer instead of defensive inline imports in knowledge_agent.py, runbook_generator.py, and knowledge_base.py. Also update test for two ChatOpenAI instances in SRELangGraphAgent.
Patch at usage location (langgraph_agent) not definition location
abrookins
changed the title
Problem(s)
The current tool interface is requires a Python class (a "tool provider") to define available tools and give them categories. For example, a
GitHubIssuesToolProvidermight define asearch_tickets()tool and categorize it as anissuestool.This works for hand-coded tool wrappers, but it poses problems if, later, we allow the user to arbitrarily add any MCP server. Doing so would require the user to write at least one custom tool provider for that MCP server. However, previous design choice meant providers were 1:1 with tool categories, so the user would need one tool provider per category of tools the MCP server exposes. We should rather have one tool provider (ideally constructed automatically) for an MCP server, not seveal tool providers, one for each tool category.
There was also some overlapping functionality between our tool schema abstractions and LangChain and some oddities worth cleaning up.
Solution
This PR breaks apart and remixes some of the tool provider-related code, such that a tool provider can provide tools for multiple categories. We also lean into relying on LangChain's tool abstractions, reducing the boilerplate necessary for tool schemas.
Changes