Add health filtering to agent_tools() method
#32
Conversation
Walkthroughagent_tools gained parameters 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (2)src/mcpd/mcpd_client.py (2)
tests/unit/test_mcpd_client.py (2)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_mcpd_client.py (1)
203-227: Add missingserver_healthmock to prevent implicit HTTP callsThe two tests at lines 203–227 are missing the
@patch.object(McpdClient, "server_health")decorator that all similar tests in this file include. Sinceagent_tools()defaults tocheck_health=True, both tests will invoke_get_healthy_servers(), which calls the unmockedserver_health()method and triggers real HTTP requests.This contradicts the established pattern used in every other
agent_toolstest (test_agent_tools,test_agent_tools_filter_by_single_server,test_agent_tools_filter_by_multiple_servers, etc.), all of which properly mockserver_health.Apply either fix suggested:
- Add
@patch.object(McpdClient, "server_health")to both tests and setmock_health.return_value = {}(consistent with existing tests); or- Call
client.agent_tools(servers=["nonexistent"], check_health=False)andclient.agent_tools(servers=[], check_health=False)respectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/mcpd/mcpd_client.py(5 hunks)tests/unit/test_mcpd_client.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (3)
McpdClient(56-882)agent_tools(370-466)servers(216-263)tests/conftest.py (1)
client(51-52)
🪛 Ruff (0.14.4)
src/mcpd/mcpd_client.py
370-370: Boolean-typed positional argument in function definition
(FBT001)
370-370: Boolean default positional argument in function definition
(FBT002)
483-483: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
tests/unit/test_mcpd_client.py (1)
129-151: Health-filtering tests look solid and cover key scenariosThe new tests around
agent_toolsand health (filters_by_health_default,no_health_check_when_disabled,all_servers_unhealthy, combined server/health filtering, and health-check exception) give good coverage of the intended behaviours, including the “exception ⇒ empty result” path.This will make future changes to the health logic much safer.
Also applies to: 152-201, 252-362
- Add check_health parameter to filter tools by server health status - Defaults to True for best performance and JavaScript SDK parity - Add _get_healthy_servers() helper method for batch health checking - Silently skip unhealthy servers (timeout, unreachable, unknown) - Add comprehensive unit tests for health filtering scenarios - Update existing tests to mock server_health - Update README with health filtering examples - Maintains full backward compatibility via check_health=False option
peteski22
force-pushed
the
peteski22/agent-tools-health-filtering
branch
from
69c1089 to
8d1257c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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)
405-411: Update docstring to remove ServerNotFoundError from Raises sectionThe docstring (line 409) documents
ServerNotFoundErroras a raised exception, but the implementation (line 459) catches and silently ignores bothServerNotFoundErrorandServerUnhealthyError. RemoveServerNotFoundError: If a server becomes unavailable during tool retrieval.from theRaises:section, as this behaviour is already documented in theArgs:section (line 389: "Non-existent server names are silently ignored").
♻️ Duplicate comments (1)
README.md (1)
93-104: Examples align with health‑filtering semantics; API bullet still misses keyword‑only markerThe “Agentic Usage” examples for
agent_toolsnow correctly demonstrate default health filtering,servers=[...], andcheck_health=False, which matches the current implementation.In the API section, the bullet uses:
client.agent_tools(servers: list[str] | None = None, check_health: bool = True) -> list[Callable]The actual signature is keyword‑only for
check_health(..., *, check_health: bool = True, ...). For accuracy and to encourage clearer call sites, consider reflecting that in the docs:client.agent_tools(servers: list[str] | None = None, *, check_health: bool = True) -> list[Callable]Also applies to: 148-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)src/mcpd/mcpd_client.py(4 hunks)tests/conftest.py(1 hunks)tests/unit/test_mcpd_client.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcpd/mcpd_client.py (2)
src/mcpd/exceptions.py (2)
ServerNotFoundError(105-132)ServerUnhealthyError(135-167)src/mcpd/function_builder.py (1)
create_function_from_schema(119-207)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (2)
agent_tools(370-467)servers(216-263)tests/conftest.py (3)
client(51-52)tools_side_effect(61-82)side_effect(77-78)
🪛 Ruff (0.14.4)
tests/conftest.py
76-76: Missing return type annotation for private function _create_side_effect
(ANN202)
77-77: Missing return type annotation for private function side_effect
(ANN202)
tests/unit/test_mcpd_client.py
228-228: Unused method argument: mock_tools
(ARG002)
🔇 Additional comments (2)
src/mcpd/mcpd_client.py (1)
370-467: Health‑first agent_tools flow and helper structure look solidThe refactored
agent_toolsnow derives the server list, applies_get_healthy_serverswhencheck_health=True, and only then fetches tools per server, which matches the documented/default health‑filtering semantics and avoids calling/toolson servers already known to be unhealthy. ReusingHealthStatus.is_healthyinside_get_healthy_serverskeeps status logic centralised. Overall, the control flow and error handling here look correct and coherent with the tests and README.Also applies to: 469-493
tests/unit/test_mcpd_client.py (1)
129-207: Health‑aware agent_tools test coverage is comprehensive and well factoredThe updated tests around
agent_toolscover the key behaviours: default health filtering, explicitservers=[...], the no‑servers case, disabling health checks, all‑unhealthy servers, combining server and health filters, and exception propagation fromserver_health. Using thetools_side_effectfixture keeps per‑server tool data clear and reduces duplication. This suite gives good confidence in the new health‑filtering logic.Also applies to: 238-390
peteski22
force-pushed
the
peteski22/agent-tools-health-filtering
branch
from
831e3b1 to
594c8a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)src/mcpd/mcpd_client.py(4 hunks)tests/conftest.py(2 hunks)tests/unit/test_mcpd_client.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcpd/mcpd_client.py (2)
src/mcpd/exceptions.py (2)
ServerNotFoundError(105-132)ServerUnhealthyError(135-167)src/mcpd/function_builder.py (1)
create_function_from_schema(119-207)
tests/unit/test_mcpd_client.py (2)
src/mcpd/mcpd_client.py (3)
McpdClient(56-880)agent_tools(370-466)servers(216-263)tests/conftest.py (3)
client(52-53)tools_side_effect(62-83)side_effect(78-79)
🔇 Additional comments (6)
src/mcpd/mcpd_client.py (1)
370-467: Health-awareagent_toolsimplementation looks consistent and robustThe new signature, default
check_health=True, and the flow of “discover servers → optionally filter via_get_healthy_servers→ fetch tools per server with targeted exception handling” align well with the docstring, README, and tests. Using a keyword-only boolean and delegating health logic to_get_healthy_serverskeeps the API clear and avoids boolean-trap issues; the per-server try/except forServerNotFoundError/ServerUnhealthyErroris a sensible resilience choice.tests/conftest.py (1)
1-1:tools_side_effectfixture cleanly abstracts per-server tools mockingThe new fixture neatly packages the
tools()side-effect pattern with clear typing and documentation, and keeps tests aligned with the realtools(server_name=...) -> list[dict]shape. This improves readability and reuse across the agent-tools tests.Also applies to: 61-83
tests/unit/test_mcpd_client.py (3)
129-268: Comprehensive coverage for default and server-filteredagent_toolspathsThe tests around
test_agent_tools, single/multiple server filtering, explicit empty server list, and omittedserversparameter collectively exercise the main success paths foragent_tools, including interaction withserver_healthandcreate_function_from_schema. Using the sharedtools_side_effectkeeps the mockedtools()contract realistic and the expectations (counts and arguments) match the implementation.
269-348: Health-filtering behaviour and check-health toggle are well exercisedThe tests for default health filtering, disabling health checks, and the “all servers unhealthy” case accurately capture the intended semantics: only healthy servers contribute tools when
check_health=True, andserver_healthis completely bypassed whencheck_health=False. Assertions on call counts (mock_health.assert_not_called,mock_create.assert_not_called) make these guarantees explicit and future-proof.
349-395: Edge cases for mixed health and health-check failures are handled correctlyThe combined server/health filtering and
test_agent_tools_health_check_exceptionensure that:
- Only the intersection of requested and healthy servers is used, and unhealthy ones are skipped.
- Exceptions from
server_health(via_get_healthy_servers) propagate rather than being silently swallowed.This aligns with the documented contract that
McpdError(or other exceptions from health retrieval) will bubble out when health checking is enabled.README.md (1)
93-104:agent_toolsdocumentation now matches the implementation and testsThe updated Agentic Usage examples and API bullet correctly reflect the new signature and default health-filtering semantics (
serversplus keyword-onlycheck_health, healthy-only by default,check_health=Falseto include all servers). This keeps users’ expectations aligned with the actual runtime behaviour.Also applies to: 148-148
peteski22
force-pushed
the
peteski22/agent-tools-health-filtering
branch
from
594c8a8 to
61a7987
Compare
peteski22
force-pushed
the
peteski22/agent-tools-health-filtering
branch
from
61a7987 to
c43896d
Compare
aittalam
left a comment
aittalam
left a comment
There was a problem hiding this comment.
Thanks @peteski22 !
I have left one comment but it's more to test my own understanding... 😅 Your changes LGTM, especially knowing that follow-up PRs are already planned 🙏
check_healthparameter to filter tools by server health statusTruefor best performance and JavaScript SDK parity_get_healthy_servers()helper method for batch health checkingcheck_health=FalseoptionSummary by CodeRabbit
New Features
Documentation
Tests