agent_tools: allow filtering and improved caching behaviour #34
Conversation
WalkthroughAdded tool-name filtering and a cache-refresh option to agent tool discovery; introduced TOOL_SEPARATOR and per-function metadata ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/test_mcpd_client.py (2)
157-190: Fix missing mock setup causing pipeline failure.The test at Line 242 is failing because
client.agent_tools(servers=["nonexistent"])callsself.servers()internally, which isn't mocked in this test. The test needs the same mock setup as other tests.Apply this diff to add the missing mocks:
+ @patch.object(McpdClient, "servers") @patch.object(McpdClient, "tools") - def test_agent_tools_filter_by_single_server( - self, mock_health, mock_tools, mock_servers, client, tools_side_effect - ): + @patch.object(McpdClient, "server_health") + def test_agent_tools_filter_by_single_server( + self, mock_health, mock_tools, mock_servers, client, tools_side_effect + ): """Test filtering tools by a single server name.""" mock_servers.return_value = ["server1", "server2"]
232-245: Fix missing mocks causing pipeline failure.Line 242 in this test (
client.agent_tools(servers=["nonexistent"])) triggers a call toservers()andserver_health()internally, but these methods aren't mocked.Apply this diff:
+ @patch.object(McpdClient, "servers") @patch.object(McpdClient, "tools") - def test_agent_tools_with_nonexistent_server(self, mock_tools, client, tools_side_effect): + @patch.object(McpdClient, "server_health") + def test_agent_tools_with_nonexistent_server(self, mock_health, mock_tools, mock_servers, client, tools_side_effect): """Test filtering with server that doesn't exist.""" + mock_servers.return_value = ["server1"] mock_tools.side_effect = tools_side_effect( { "server1": [{"name": "tool1", "description": "Test tool"}], } ) + mock_health.return_value = { + "server1": {"status": "ok"}, + } with patch.object(client._function_builder, "create_function_from_schema") as mock_create: result = client.agent_tools(servers=["nonexistent"])src/mcpd/mcpd_client.py (2)
496-535: Consider adding logging for schema fetch failures.The method silently continues when tool schema fetching fails for healthy servers (Lines 526-529). The TODO comment on Line 528 indicates logging should be added. This could make debugging difficult when schemas fail to load unexpectedly.
While the current behaviour (silently skipping failed servers) is reasonable, consider adding the logging as indicated in the TODO to improve observability:
except (ConnectionError, TimeoutError, AuthenticationError, ServerNotFoundError, McpdError) as e: # TODO: Add logging once logger is available # self._logger.warning( # f"Failed to retrieve tool schema for server '{server_name}': {e}" # ) continueThis would help operators understand why certain tools aren't available despite servers being healthy.
537-565: Consider improving unhealthy server handling with logging.The
is_validhelper (Lines 555-563) has TODO comments indicating that logging should be added for non-existent and unhealthy servers. Without logging, users may not understand why their requested servers aren't included in the results.Consider implementing the logging indicated in the TODOs:
def is_valid(name: str) -> bool: health = health_map.get(name) if not health: # TODO: Add logging once logger is available # self._logger.warning(f"Skipping non-existent server '{name}'") return False status = health.get("status") if not HealthStatus.is_healthy(status): # TODO: Add logging once logger is available # self._logger.warning( # f"Skipping unhealthy server '{name}' with status '{status}'" # ) return False return TrueThis would improve observability and help users understand filtering behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)src/mcpd/dynamic_caller.py(3 hunks)src/mcpd/function_builder.py(8 hunks)src/mcpd/mcpd_client.py(7 hunks)tests/unit/test_function_builder.py(1 hunks)tests/unit/test_mcpd_client.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/test_function_builder.py (1)
src/mcpd/function_builder.py (1)
create_function_from_schema(126-223)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (4)
McpdClient(70-1006)agent_tools(384-494)servers(230-277)tools(279-341)tests/conftest.py (3)
client(52-53)tools_side_effect(62-83)side_effect(78-79)
src/mcpd/mcpd_client.py (2)
src/mcpd/function_builder.py (2)
FunctionBuilder(31-627)get_cached_functions(621-627)src/mcpd/exceptions.py (5)
ConnectionError(62-80)TimeoutError(275-305)AuthenticationError(83-102)ServerNotFoundError(105-132)McpdError(8-59)
🪛 GitHub Actions: Unit Tests
tests/unit/test_mcpd_client.py
[error] 242-242: Cannot connect to mcpd daemon at http://localhost:8090: HTTPConnectionPool(host='localhost', port=8090): Max retries exceeded with url: /api/v1/servers (Connection refused).
[error] 605-605: Cannot connect to mcpd daemon at http://localhost:8090: HTTPConnectionPool(host='localhost', port=8090): Max retries exceeded with url: /api/v1/servers (Connection refused).
[error] 631-631: Cannot connect to mcpd daemon at http://localhost:8090: HTTPConnectionPool(host='localhost', port=8090): Max retries exceeded with url: /api/v1/servers (Connection refused).
src/mcpd/mcpd_client.py
[error] 262-262: Cannot connect to mcpd daemon at http://localhost:8090: Connection refused (used by tests validating agent_tools when servers/tools are queried).
[error] 492-492: Cannot connect to mcpd daemon at http://localhost:8090: Connection refused (used by tests validating agent_tools when filtering by nonexistent tool).
[error] 631-631: Cannot connect to mcpd daemon at http://localhost:8090: Connection refused (used by tests validating agent_tools with empty tools list).
🪛 Ruff (0.14.5)
src/mcpd/mcpd_client.py
48-48: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in __call__
(ANN401)
src/mcpd/function_builder.py
206-206: Use of exec detected
(S102)
🔇 Additional comments (21)
README.md (3)
102-112: LGTM! Clear examples of new filtering capabilities.The examples effectively demonstrate both raw tool name filtering (cross-cutting across servers) and qualified tool name filtering (server-specific). The combination example shows how both filters can work together.
171-173: LGTM! API documentation accurately reflects the new signature.The documentation clearly describes the new
toolsparameter with support for both raw and qualified names, and therefresh_cacheparameter for forcing regeneration.
125-137: Caching semantics verified as accurate – no documentation changes required.Verification confirms the implementation and documentation are correct:
- Cache is built from all healthy servers on first call (_agent_tools lines 521–535)
- Filter parameters (servers, tools) are applied post-cache via _filter_agent_tools (line 494)
- Calling
agent_tools(servers=['time'])does build the full cache, then filters it- The README documentation at line 404 correctly states "regardless of filter parameters," which accurately describes this behaviour
The existing documentation is sufficient; no clarification additions are needed.
src/mcpd/dynamic_caller.py (3)
13-13: LGTM! Proper import for type hints.Using
Callablefromcollections.abcis the recommended approach for Python 3.9+.
17-17: LGTM! Consistent use of TOOL_SEPARATOR constant.Importing the constant ensures consistent naming across the codebase.
114-166: LGTM! Improved type hints and consistent naming.The return type is now properly annotated as
Callable(Line 114), and the function name uses theTOOL_SEPARATORconstant (Line 166) for consistency with the new naming convention.tests/unit/test_mcpd_client.py (1)
420-674: Excellent test coverage for tool filtering.The test suite comprehensively covers:
- Raw tool name filtering (cross-cutting across servers)
- Prefixed/qualified tool name filtering (server-specific)
- Mixed format filtering
- Combining server and tool filters
- Edge cases: empty lists, nonexistent tools, tool names with double underscores
This thorough testing ensures the filtering logic is robust.
tests/unit/test_function_builder.py (3)
249-256: LGTM! Verifies _server_name metadata attribute.The test confirms that generated functions have the
_server_nameattribute with the correct value.
258-265: LGTM! Verifies _tool_name metadata attribute.The test confirms that generated functions have the
_tool_nameattribute with the correct value.
267-277: LGTM! Verifies metadata preservation with special characters.The test correctly verifies that:
- Function names are sanitised (Line 274)
- Metadata attributes preserve original names (Lines 276-277)
This is important for maintaining the original server/tool identifiers while ensuring valid Python function names.
src/mcpd/function_builder.py (5)
14-14: LGTM! Proper import for type hints.Using
Callablefromcollections.abcis correct for modern Python.
23-28: LGTM! Well-documented constant with clear naming convention.The
TOOL_SEPARATORconstant is properly documented with format and examples, making the naming convention explicit and maintainable.
94-124: LGTM! Consistent use of TOOL_SEPARATOR throughout.The method correctly uses
TOOL_SEPARATORin the implementation (Line 124) and updates all documentation references to use the constant name instead of the hardcoded "__" string.
126-223: LGTM! Enhanced function generation with metadata attributes.The changes properly:
- Update return type to
Callable[..., Any](Line 126)- Add metadata attributes to created functions (Lines 199-201)
- Add metadata attributes to cached instances (Lines 209-211)
- Update cache key to use TOOL_SEPARATOR (Line 181)
- Update documentation to reflect metadata attributes (Lines 153-154)
The metadata enables the new filtering capabilities in
agent_tools().
621-627: LGTM! Enables cache retrieval for agent_tools.The new
get_cached_functions()method provides access to cached functions, which is essential for the caching behaviour described in the documentation. The method correctly creates new instances from cached templates using the stored factory function.src/mcpd/mcpd_client.py (6)
16-16: LGTM! Protocol import for type safety.Adding
Protocolenables the internal_AgentFunctiontype for better type checking.
31-31: LGTM! Consistent import of TOOL_SEPARATOR.Importing the constant ensures consistent use across the client implementation.
37-48: LGTM! Well-designed internal Protocol for type safety.The
_AgentFunctionProtocol correctly defines the expected interface for generated functions with:
__name__: Function name_server_name: Server metadata_tool_name: Tool metadata__call__: Callable signatureThe comment explains this is internal and not exposed in the public API, which is appropriate.
384-494: LGTM! Well-documented API with clear behaviour.The updated
agent_tools()method:
- Properly documents the new
toolsandrefresh_cacheparameters (Lines 384-423)- Provides comprehensive examples covering various filtering scenarios (Lines 440-483)
- Clearly explains the caching behaviour (Lines 402-406)
- Delegates to helper methods for clean separation of concerns (Lines 489-494)
The implementation is clean and maintainable.
592-618: LGTM! Clean filtering implementation with proper separation.The
_filter_agent_tools()method correctly:
- Applies server filtering if specified (Lines 611-612)
- Applies tool filtering if specified (Lines 615-616)
- Returns the filtered list
The sequential filtering approach is clear and maintainable.
567-590: No issues identified — edge case handling is documented and working as intended.The filtering logic correctly implements the documented behaviour. The docstring (lines 575-576) explicitly states the limitation: "exact match against func.name" when
TOOL_SEPARATORis present. The implementation uses==comparisons (lines 586-588), enforcing exact matches. Partial prefix matches like["test__my"]intentionally do not match"test__my__special__tool", which is clearly documented.Test coverage (
test_agent_tools_handles_tool_name_with_double_underscore, lines 640–676) verifies both supported formats work correctly. The existing documentation is sufficient.
peteski22
force-pushed
the
peteski22/agent-tools-tool-filtering
branch
from
bf900ad to
c914126
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcpd/dynamic_caller.py (1)
114-167: Aligntool_functionmetadata with agent tool functionsYou now align
__name__with theTOOL_SEPARATORconvention, which is good. For consistency with_AgentFunction/FunctionBuilder-generated callables, consider also setting_server_nameand_tool_nameontool_function. That keepsclient.call.*functions introspectable (and potentially re-usable in the same filtering machinery) in the same way asagent_tools()functions.src/mcpd/mcpd_client.py (1)
537-566: Health filtering is clear, but logging TODOs could be fleshed out later
_get_healthy_servers()’s use ofserver_health()plusHealthStatus.is_healthy()gives a straightforward “only ok servers” filter, and silently skipping missing/unhealthy servers matches the broader API behaviour. The TODO comments about logging for non-existent / unhealthy servers are useful markers; once a logger is wired in, turning those into real warnings will aid debugging without changing semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)src/mcpd/dynamic_caller.py(3 hunks)src/mcpd/function_builder.py(8 hunks)src/mcpd/mcpd_client.py(7 hunks)tests/unit/test_function_builder.py(1 hunks)tests/unit/test_mcpd_client.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcpd/dynamic_caller.py (1)
src/mcpd/exceptions.py (1)
ToolNotFoundError(170-201)
tests/unit/test_function_builder.py (1)
src/mcpd/function_builder.py (1)
create_function_from_schema(126-223)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (3)
agent_tools(384-494)servers(230-277)tools(279-341)tests/conftest.py (3)
client(52-53)tools_side_effect(62-83)side_effect(78-79)
🪛 Ruff (0.14.5)
src/mcpd/function_builder.py
206-206: Use of exec detected
(S102)
src/mcpd/mcpd_client.py
48-48: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in __call__
(ANN401)
🔇 Additional comments (6)
src/mcpd/function_builder.py (2)
23-28: Consistent tool naming viaTOOL_SEPARATORlooks goodCentralising the separator and using
_safe_name()for both server and tool parts in_function_name()gives a clear, predictable qualified name while keeping function names valid identifiers, and the docstring accurately documents the format and introspection story.Also applies to: 94-125
126-155: Metadata on generated functions is well-structuredUsing the raw
server_nameand schema"name"for_server_name/_tool_namewhile keeping__name__sanitised avoids cache-key collisions and preserves original identifiers for filtering. Propagating the same metadata into cached instances viacreate_function_instance()keeps behaviour consistent across first-build and cache hits.Also applies to: 179-182, 199-218
README.md (1)
102-113: Documentation matches new filtering and caching semanticsThe added
agent_toolsexamples for tool-name filtering (raw and qualified) and the IMPORTANT note about cache behaviour /refresh_cacheline up with the new implementation inMcpdClient.agent_tools()and_agent_tools(). The API section signature is also in sync.Also applies to: 125-137, 171-174
tests/unit/test_mcpd_client.py (1)
157-415: Server-filter and health-filter tests match the new flowThe new
agent_toolstests aroundservers=[...](single, multiple, non-existent, empty list) and combined with health status correctly mirror_get_healthy_servers()+_filter_agent_tools()semantics, including the “build full cache, then filter” behaviour described in the comments. This gives good confidence in server/health interaction without over-coupling to implementation details.src/mcpd/mcpd_client.py (2)
384-407: agent_tools caching and filtering semantics are coherent with the designThe refactored
agent_tools()cleanly separates concerns: cache management (refresh_cache→clear_agent_tools_cache()), building or reusing the underlying tool set via_agent_tools(), and applying server/tool filters via_filter_agent_tools(). UsingFunctionBuilder.get_cached_functions()to short-circuit means health checks and schema fetches occur only on cache build, which aligns with the README’s “subsequent calls return cached functions without refetching schemas” guidance; callers who care about new tools or health changes can opt intorefresh_cache=Trueorclear_agent_tools_cache().Also applies to: 425-439, 489-495, 496-535
567-590: Tool-filter logic matches the documented matching rules, including separator edge cases
_matches_tool_filter()correctly implements the two modes laid out in the docstring: raw tool-name matching when the filter string lacksTOOL_SEPARATOR, and exact qualified-name-or-raw matching when it contains the separator. This also handles the “tool name itself contains__” case by falling back to_tool_namecomparison, which is exercised in the new tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcpd/mcpd_client.py (1)
384-494: Enhanced agent_tools() API with filtering and cache control.The extended API signature is well-designed:
serversparameter enables filtering by server namestoolsparameter supports both raw and qualified tool name filteringrefresh_cacheparameter allows forcing cache regenerationThe implementation correctly delegates to
_agent_tools()for cache management and_filter_agent_tools()for filtering, maintaining a clean separation of concerns.However, the docstring contains inaccuracies noted in a previous review that should be addressed:
- Line 431 references a non-existent
_schemaattribute (only_server_nameand_tool_nameexist)- Lines 437-438 state
McpdErroris raised for failures retrieving tool definitions, but_agent_tools()catches and swallows these exceptions per-serverClarify the docstring:
Apply this diff to correct the Returns section (remove
_schemareference):Returns: A list of callable functions, one for each matching tool from healthy servers. Each function has the following attributes: - __name__: The tool's qualified name (e.g., "time__get_current_time") - __doc__: The tool's description - - _schema: The tool's JSON Schema definition - _server_name: The server hosting this tool (original name) - _tool_name: The tool's name (original name)And update the Raises section to clarify exception behaviour:
Raises: ConnectionError: If unable to connect to the mcpd daemon. TimeoutError: If requests to the daemon time out. AuthenticationError: If API key authentication fails. - McpdError: If unable to retrieve server health status or generate functions. - Servers for which tool schemas cannot be retrieved will be ignored. + McpdError: If unable to retrieve server health status or compile function templates. + Individual server failures when fetching tool schemas are caught and + those servers are skipped silently.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/mcpd/mcpd_client.py(7 hunks)tests/unit/test_function_builder.py(2 hunks)tests/unit/test_mcpd_client.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/test_function_builder.py (1)
src/mcpd/function_builder.py (1)
create_function_from_schema(126-223)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (4)
McpdClient(70-1007)agent_tools(384-494)servers(230-277)tools(279-341)tests/conftest.py (3)
client(52-53)tools_side_effect(62-83)side_effect(78-79)
src/mcpd/mcpd_client.py (2)
src/mcpd/function_builder.py (2)
FunctionBuilder(31-627)get_cached_functions(621-627)src/mcpd/exceptions.py (5)
ConnectionError(62-80)TimeoutError(275-305)AuthenticationError(83-102)ServerNotFoundError(105-132)McpdError(8-59)
🪛 Ruff (0.14.5)
tests/unit/test_mcpd_client.py
714-714: Missing return type annotation for private function create_time_func
(ANN202)
714-714: Unused function argument: annotations
(ARG001)
717-717: Missing return type annotation for private function create_math_func
(ANN202)
717-717: Unused function argument: annotations
(ARG001)
src/mcpd/mcpd_client.py
48-48: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
48-48: Dynamically typed expressions (typing.Any) are disallowed in __call__
(ANN401)
🔇 Additional comments (10)
tests/unit/test_function_builder.py (2)
144-146: LGTM! Metadata assertions added to caching test.The caching test now correctly asserts that both cached function instances preserve the
_server_nameand_tool_namemetadata attributes. This addresses the previous review suggestion and ensures regressions in metadata propagation are caught.
252-280: LGTM! Comprehensive metadata attribute tests.The three new test methods provide excellent coverage:
- Verification that
_server_nameattribute exists and matches the provided server name- Verification that
_tool_nameattribute exists and matches the provided tool name- Verification that special characters are sanitized in function names whilst metadata retains original names
These tests align well with the metadata propagation behaviour introduced in
FunctionBuilder.create_function_from_schema().tests/unit/test_mcpd_client.py (3)
157-413: LGTM! Comprehensive server filtering tests.The new test methods thoroughly cover server filtering scenarios:
- Single server filtering
- Multiple server filtering
- Non-existent server handling
- Empty server list handling
- Combined with health filtering
The tests correctly set up mock metadata (
_server_name,_tool_name) and verify that the cache is built from all healthy servers whilst filtering is applied to the returned results. This validates the separation between cache construction and result filtering.
434-696: LGTM! Excellent tool filtering test coverage.The tool filtering tests comprehensively cover:
- Raw tool name filtering (cross-cutting across servers)
- Prefixed/qualified tool name filtering (specific server+tool)
- Mixed format filtering
- Combined server and tool filtering
- Non-existent tool handling
- Empty tool list handling
- Tool names containing the separator (
__)The tests correctly exercise both the raw-name and prefixed-name matching logic in
_matches_tool_filter(), including the edge case where tool names themselves containTOOL_SEPARATOR. This should prevent regressions in the filtering logic.
700-743: Good addition: cache hit test validates discovery methods are skipped.This test addresses the previous review suggestion by pre-populating the cache and verifying that
agent_tools()returns cached functions without invokingservers(),tools(), orserver_health(). This ensures the cache-hit path correctly short-circuits the discovery flow.The test creates mock functions with required metadata and uses factory functions to simulate the cache structure, which correctly models the actual caching behaviour.
src/mcpd/mcpd_client.py (5)
37-49: Protocol definition is appropriate for dynamically-shaped functions.The
_AgentFunctionProtocol provides good internal typing for generated tool functions with metadata attributes (__name__,_server_name,_tool_name) and a callable signature. The use ofAnyin the__call__signature is pragmatically reasonable for dynamically-shaped agent functions, where parameter types vary based on tool schemas.The static analysis tool flags ANN401 for
Any, but suppressing this for an internal protocol is acceptable given the dynamic nature of MCP tool schemas. The protocol is marked as internal and not exposed in the public API, which is the right approach.
496-536: LGTM! Cache management and healthy server filtering.The
_agent_tools()method correctly implements the caching strategy:
- Returns cached functions immediately if available (line 518-520)
- Builds cache from all healthy servers on first call
- Catches and continues on per-server schema retrieval failures (lines 527-530)
The exception handling is appropriate: connection, timeout, authentication, server-not-found, and general McpdError exceptions are caught per-server, allowing the method to continue with remaining healthy servers. This aligns with the graceful degradation approach.
The TODO comments (lines 529, 560, 564) noting where logging should be added are helpful markers for future observability improvements.
538-566: LGTM! Healthy server filtering logic.The
_get_healthy_servers()method correctly filters to servers with 'ok' health status using theHealthStatus.is_healthy()helper. The nestedis_valid()function properly handles both non-existent servers (line 559-561) and unhealthy servers (line 563).The TODO comments indicate where logging should be added for skipped servers, which is good practice for future debugging.
568-591: LGTM! Tool filter matching logic handles both raw and qualified names.The
_matches_tool_filter()method correctly implements the dual matching strategy:
- If filter item contains no
TOOL_SEPARATOR, match against_tool_name(raw name)- If filter item contains
TOOL_SEPARATOR, check both__name__(qualified) and_tool_name(handles edge case where tool name itself contains separator)The logic at lines 586-590 correctly uses a generator expression with
any()to check if the function matches any filter item. The fallback to raw name matching (line 589) ensures tools with__in their names can be matched by qualified names.
593-619: LGTM! Straightforward filtering implementation.The
_filter_agent_tools()method applies server and tool filters sequentially, which is correct and efficient. The conditional filtering (lines 612-613, 616-617) ensuresNoneparameters are treated as "no filter" rather than "filter to nothing", which matches the documented API behaviour.
| def create_time_func(annotations): | ||
| return mock_func1 | ||
|
|
||
| def create_math_func(annotations): | ||
| return mock_func2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Address static analysis hints for factory functions.
The static analysis tool correctly identifies that the factory functions create_time_func and create_math_func are missing return type annotations and have unused annotations parameters. Whilst these are internal test helpers and the issues are minor, addressing them would improve code quality.
Apply this diff to add type hints and mark unused parameters:
- def create_time_func(annotations):
+ def create_time_func(annotations: dict[str, Any]) -> Mock: # noqa: ARG001
return mock_func1
- def create_math_func(annotations):
+ def create_math_func(annotations: dict[str, Any]) -> Mock: # noqa: ARG001
return mock_func2Don't forget to import Any from typing at the top of the file if it's not already imported.
Based on static analysis hints.
🧰 Tools
🪛 Ruff (0.14.5)
714-714: Missing return type annotation for private function create_time_func
(ANN202)
714-714: Unused function argument: annotations
(ARG001)
717-717: Missing return type annotation for private function create_math_func
(ANN202)
717-717: Unused function argument: annotations
(ARG001)
🤖 Prompt for AI Agents
In tests/unit/test_mcpd_client.py around lines 714 to 718, the factory functions
create_time_func and create_math_func lack return type annotations and declare
an unused parameter `annotations`; add proper typing and mark the unused
parameter as such: change signatures to accept annotations: Any (import Any from
typing at top if missing) and annotate the return type to the callable type
(e.g., Callable[..., Any] or the specific mock function type used), and either
rename the parameter to _annotations or add a leading underscore to indicate it
is intentionally unused.
Summary by CodeRabbit
New Features
Documentation
Behaviour
Tests
✏️ Tip: You can customize this high-level summary in your review settings.