Actually use FastMCP#110
Conversation
WalkthroughUpdated minimum fastmcp version and refactored server to use fastmcp.FastMCP, header-based token extraction via get_http_headers, added StripSessionIDMiddleware, unified http_app transport, simplified tool listing, and adapted tests to a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTPApp as HTTP App (streamable-http)
participant Middleware as StripSessionIDMiddleware
participant Headers as get_http_headers
participant FastMCP as FastMCP
Client->>HTTPApp: HTTP tool call (headers + payload)
HTTPApp->>Middleware: on_call_tool(context)
Middleware->>Headers: get_http_headers(include_all=true)
Headers-->>Middleware: headers (authorization, ocm-offline-token)
note right of Middleware: Remove `session_id` from context
Middleware->>FastMCP: call tool (context without session_id, tokens)
FastMCP-->>Middleware: ToolResult
Middleware-->>HTTPApp: ToolResult
HTTPApp-->>Client: HTTP response (content/text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
![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
🧹 Nitpick comments (7)
pyproject.toml (1)
9-9: FastMCP bump aligns with code changes; consider upper-bounding and confirm transitivemcptypes availability
- Optional: add an upper bound to avoid unexpected v3 breaking changes.
- Verify that
fastmcp>=2.12.3brings inmcp(formcp.types.TextContentin tests) transitively in your environment.Apply this diff if you want an upper bound:
- "fastmcp>=2.12.3", + "fastmcp>=2.12.3,<3",server.py (4)
91-96: Normalize header keys to avoid case-sensitivity pitfallsHTTP headers are case-insensitive but sources may provide mixed casing. Normalize once to lower to make lookups robust.
Apply this diff:
- headers = get_http_headers(include_all=True) - token = headers.get("ocm-offline-token") + headers = {k.lower(): v for k, v in get_http_headers(include_all=True).items()} + token = headers.get("ocm-offline-token")
117-124: Same header normalization for Authorization parsingGuard against mixed-case header keys.
Apply this diff:
- headers = get_http_headers(include_all=True) - header = headers.get("authorization") + headers = {k.lower(): v for k, v in get_http_headers(include_all=True).items()} + header = headers.get("authorization")
941-943: Avoid asyncio.run() if there’s a chance this is called within a running loopIf
list_tools()is ever called from an async context (not just at startup),asyncio.run()will raise. Consider exposing an async variant or using a loop-aware helper.Would you prefer an async
list_tools_async()and have startup callasyncio.run(list_tools_async())while other callers can await?
946-963: Harden middleware against missing/None argumentsDefensively handle cases where
argumentsmight not be a dict.Apply this diff:
class StripSessionIDMiddleware(Middleware): @@ - async def on_call_tool( + async def on_call_tool( self, context: MiddlewareContext, call_next: CallNext, ) -> ToolResult: - """Strip session_id from tool calls""" - if "session_id" in context.message.arguments: - del context.message.arguments["session_id"] + """Strip session_id from tool calls.""" + args = getattr(context.message, "arguments", None) + if isinstance(args, dict) and "session_id" in args: + del args["session_id"] return await call_next(context)tests/test_server.py (2)
83-90: Drop redundant test or parametrize
test_get_offline_token_no_requestduplicatestest_get_offline_token_not_found. Consider removing or parametrizing to keep the suite lean.Apply this diff to remove the redundant test:
- def test_get_offline_token_no_request( - self, mock_mcp_get_http_headers: dict[str, Any] - ) -> None: - """Test offline token retrieval when no request is available.""" - with patch.dict(os.environ, {}, clear=True): - with pytest.raises(RuntimeError) as exc_info: - server.get_offline_token() - assert "No offline token found" in str(exc_info.value)
101-115: Assert raise_for_status() is called in access-token testsYou call
response.raise_for_status()in production; assert it in tests to catch regressions.Example change for one test:
- mock_response = Mock() + mock_response = Mock() + mock_response.raise_for_status.return_value = None mock_response.json.return_value = {"access_token": "new-token"} mock_post.return_value = mock_response @@ result = server.get_access_token() assert result == "new-token" + mock_response.raise_for_status.assert_called_once()Apply similarly to the other access-token tests mocking
requests.post.Also applies to: 120-128, 129-154, 156-182
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.toml(1 hunks)server.py(5 hunks)tests/test_server.py(35 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_server.py (2)
server.py (16)
get_offline_token(69-98)get_access_token(101-139)cluster_info(144-173)list_clusters(178-207)cluster_events(212-236)host_events(241-273)cluster_iso_download_url(278-338)create_cluster(343-447)set_cluster_vips(571-617)install_cluster(622-651)list_versions(656-672)list_operator_bundles(677-692)add_operator_bundle_to_cluster(697-730)set_host_role(831-878)cluster_credentials_download_url(735-789)set_cluster_ssh_key(883-934)service_client/assisted_service_api.py (4)
list_clusters(146-156)create_cluster(242-278)install_cluster(367-382)add_operator_bundle_to_cluster(419-446)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (5)
server.py (3)
19-22: Switch to FastMCP imports looks correctUsing
fastmcp.FastMCP,get_http_headers, and middleware types is consistent with the new usage.
36-36: FastMCP initialization is appropriate
mcp = FastMCP("AssistedService")is a clean, minimal init for registering tools.
966-968: Transport selection change: double-check environment support
streamable-httprequires the newer FastMCP HTTP transport. Ensure deploy/runtime aligns with this transport.If needed, I can add a startup self-check endpoint that reports the selected transport and registered tools.
tests/test_server.py (2)
31-36: Header mocking fixture is clean and focusedPatching
server.get_http_headersto a shared mapping keeps tests simple and explicit.
12-12: Confirm availability of mcp.types.TextContent at runtimeTests import
TextContentfrommcp.types. Ensure your dependency set (viafastmcp>=2.12.3) guaranteesmcpis installed in CI/production.Would you like me to add an import self-check in a small smoke test?
| result = await server.set_host_role.run( | ||
| {"host_id": host_id, "cluster_id": infraenv_id, "role": role} | ||
| ) | ||
|
|
||
| assert result == host.to_str() | ||
| assert cast(TextContent, result.content[0]).text == host.to_str() | ||
| mock_inventory_client.update_host.assert_called_once_with( | ||
| host_id, infraenv_id, host_role=role | ||
| ) |
There was a problem hiding this comment.
Incorrect variable passed for cluster_id in set_host_role test
The test passes infraenv_id as cluster_id, weakening coverage. Pass a real cluster_id and assert the list call uses it.
Apply this diff:
@@
- host_id = "test-host-id"
- infraenv_id = "test-infraenv-id"
+ host_id = "test-host-id"
+ cluster_id = "test-cluster-id"
+ infraenv_id = "test-infraenv-id"
@@
- result = await server.set_host_role.run(
- {"host_id": host_id, "cluster_id": infraenv_id, "role": role}
- )
+ result = await server.set_host_role.run(
+ {"host_id": host_id, "cluster_id": cluster_id, "role": role}
+ )
@@
- mock_inventory_client.update_host.assert_called_once_with(
+ mock_inventory_client.list_infra_envs.assert_called_once_with(cluster_id)
+ mock_inventory_client.update_host.assert_called_once_with(
host_id, infraenv_id, host_role=role
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = await server.set_host_role.run( | |
| {"host_id": host_id, "cluster_id": infraenv_id, "role": role} | |
| ) | |
| assert result == host.to_str() | |
| assert cast(TextContent, result.content[0]).text == host.to_str() | |
| mock_inventory_client.update_host.assert_called_once_with( | |
| host_id, infraenv_id, host_role=role | |
| ) | |
| host_id = "test-host-id" | |
| cluster_id = "test-cluster-id" | |
| infraenv_id = "test-infraenv-id" | |
| result = await server.set_host_role.run( | |
| {"host_id": host_id, "cluster_id": cluster_id, "role": role} | |
| ) | |
| assert cast(TextContent, result.content[0]).text == host.to_str() | |
| mock_inventory_client.list_infra_envs.assert_called_once_with(cluster_id) | |
| mock_inventory_client.update_host.assert_called_once_with( | |
| host_id, infraenv_id, host_role=role | |
| ) |
🤖 Prompt for AI Agents
In tests/test_server.py around lines 839 to 846, the test incorrectly passes
infraenv_id as the cluster_id when calling server.set_host_role.run, reducing
test coverage; change the call to pass the real cluster_id variable (e.g.,
{"host_id": host_id, "cluster_id": cluster_id, "role": role}) and update the
mock assertion to expect mock_inventory_client.update_host to be called with
(host_id, cluster_id, host_role=role) so the test validates the actual cluster
id is used.
Previously we were installing the package but using the official SDK for of FastMCP 1.0. Using FastMCP directly gives us a lot more features should we need them.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server.py (4)
91-96: Harden header lookup: handle missing request context and case-insensitive headersIf no HTTP request context exists,
get_http_headers()may raise. Also, header keys are case-insensitive; normalize to lowercase before lookup.Apply:
- headers = get_http_headers(include_all=True) - token = headers.get("ocm-offline-token") - if token: - log.debug("Found offline token in request headers") - return token + try: + raw_headers = get_http_headers(include_all=True) + headers = {k.lower(): v for k, v in (raw_headers or {}).items()} + except Exception: + headers = {} + token = headers.get("ocm-offline-token") + if token: + log.debug("Found offline token in request headers") + return token
117-124: Same here: robust, case-insensitive Authorization parsingNormalize header names and be resilient to missing request context.
- headers = get_http_headers(include_all=True) - header = headers.get("authorization") - if header is not None: - parts = header.split() - if len(parts) == 2 and parts[0].lower() == "bearer": - log.debug("Found access token in authorization header") - return parts[1] + try: + raw_headers = get_http_headers(include_all=True) + headers = {k.lower(): v for k, v in (raw_headers or {}).items()} + except Exception: + headers = {} + header = headers.get("authorization") + if header: + parts = header.split() + if len(parts) == 2 and parts[0].lower() == "bearer": + log.debug("Found access token in authorization header") + return parts[1]
946-963: Middleware: guard when arguments is None/not a dictAvoids errors if a tool is invoked without an arguments dict.
async def on_call_tool( self, context: MiddlewareContext, call_next: CallNext, ) -> ToolResult: """Strip session_id from tool calls""" - if "session_id" in context.message.arguments: - del context.message.arguments["session_id"] + args = context.message.arguments + if isinstance(args, dict) and "session_id" in args: + del args["session_id"] return await call_next(context)
966-971: Make server port configurableExpose port via env for flexibility in containerized deployments.
mcp.add_middleware(StripSessionIDMiddleware()) app = mcp.http_app(transport="streamable-http") initiate_metrics(list_tools()) app.add_route("/metrics", metrics) - uvicorn.run(app, host="0.0.0.0") + uvicorn.run(app, host="0.0.0.0", port=int(os.environ.get("PORT", "8000")))tests/test_server.py (2)
185-197: “no request context” test still uses the header fixtureThis test claims no request context, but the fixture patches
get_http_headersto return{}, not to simulate absence/exception. Make it realistic by removing the fixture and makingget_http_headersraise.- def test_get_access_token_no_request_context( - self, mock_mcp_get_http_headers: dict[str, Any] - ) -> None: + def test_get_access_token_no_request_context(self) -> None: """Test access token retrieval when no request context is available.""" - with patch.object(server, "get_offline_token", return_value="offline-token"): - with patch("requests.post") as mock_post: - mock_response = Mock() - mock_response.json.return_value = {"access_token": "new-token"} - mock_post.return_value = mock_response - - result = server.get_access_token() - assert result == "new-token" + with patch.object(server, "get_http_headers", side_effect=RuntimeError("no ctx")): + with patch.object(server, "get_offline_token", return_value="offline-token"): + with patch("requests.post") as mock_post: + mock_response = Mock() + mock_response.json.return_value = {"access_token": "new-token"} + mock_post.return_value = mock_response + result = server.get_access_token() + assert result == "new-token"
946-970: Add middleware unit coverage (optional)Consider a small unit test for
StripSessionIDMiddlewareto assert it dropssession_idbefore tool execution.I can draft a minimal test that invokes
on_call_toolwith a stubMiddlewareContextand verifies the forwarded arguments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.toml(1 hunks)server.py(5 hunks)tests/test_server.py(35 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_server.py (2)
server.py (16)
get_offline_token(69-98)get_access_token(101-139)cluster_info(144-173)list_clusters(178-207)cluster_events(212-236)host_events(241-273)cluster_iso_download_url(278-338)create_cluster(343-447)set_cluster_vips(571-617)install_cluster(622-651)list_versions(656-672)list_operator_bundles(677-692)add_operator_bundle_to_cluster(697-730)set_host_role(831-878)cluster_credentials_download_url(735-789)set_cluster_ssh_key(883-934)service_client/assisted_service_api.py (4)
list_clusters(146-156)create_cluster(242-278)install_cluster(367-382)add_operator_bundle_to_cluster(419-446)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (3)
tests/test_server.py (3)
30-37: Nice, focused header-mock fixtureLocalizing header manipulation via a patched
get_http_headerskeeps tests hermetic.
227-233: Accessing tool result via TextContent is correctThe
.run(payload)+result.content[0].textusage aligns with the updated FastMCP result shape.If future changes introduce non-text content, consider a small helper to extract text safely (e.g., assert
type == "text"). Do you want a shared helper function for these assertions?
830-837: Incorrect variable passed for cluster_id in set_host_role testThe test passes
infraenv_idascluster_id, weakening coverage. Pass a realcluster_idand assert the list call uses it.Apply:
@@ - host_id = "test-host-id" - infraenv_id = "test-infraenv-id" + host_id = "test-host-id" + cluster_id = "test-cluster-id" + infraenv_id = "test-infraenv-id" @@ - result = await server.set_host_role.run( - {"host_id": host_id, "cluster_id": infraenv_id, "role": role} - ) + result = await server.set_host_role.run( + {"host_id": host_id, "cluster_id": cluster_id, "role": role} + ) @@ + mock_inventory_client.list_infra_envs.assert_called_once_with(cluster_id) mock_inventory_client.update_host.assert_called_once_with( host_id, infraenv_id, host_role=role )
|
@keitwb: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Previously we were installing the package but using the official SDK for of FastMCP 1.0. Using FastMCP directly gives us a lot more features should we need them.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests