add client kwargs to proxy clients and meta to proxy tool calls#2520
add client kwargs to proxy clients and meta to proxy tool calls#2520jlowin merged 8 commits intoPrefectHQ:mainfrom
Conversation
WalkthroughProxyTool.run now obtains the request context, extracts metadata from Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/fastmcp/server/proxy.py (2)
285-290: Simplify by removing unused context parameter.The
contextparameter is immediately overwritten byget_context()on Line 290, making the parameter misleading. Sinceget_context()always retrieves the active context, the parameter serves no purpose.Apply this diff to remove the unused parameter:
async def run( self, arguments: dict[str, Any], - context: Context | None = None, ) -> ToolResult: - from fastmcp.utilities.types import find_kwarg_by_type """Executes the tool by making a call through the client.""" async with self._client: context = get_context()
292-292: Simplify meta extraction with getattr.The chained
hasattrchecks are verbose. Usegetattrwith a default value for cleaner code.Apply this diff to simplify:
- meta = dict(context.request_context.meta) if hasattr(context, 'request_context') and hasattr(context.request_context, 'meta') else None + request_context = getattr(context, 'request_context', None) + meta = dict(request_context.meta) if request_context and hasattr(request_context, 'meta') else None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/fastmcp/client/client.py(1 hunks)src/fastmcp/client/transports.py(5 hunks)src/fastmcp/mcp_config.py(2 hunks)src/fastmcp/server/proxy.py(1 hunks)src/fastmcp/utilities/mcp_config.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Python source code must be version ≥3.10 with full type annotations
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if they're shorter
Never use bareexceptin code - be specific with exception types
Files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/client.pysrc/fastmcp/server/proxy.pysrc/fastmcp/client/transports.pysrc/fastmcp/mcp_config.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T01:29:57.585Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing using in-memory transport instead of HTTP transport, unless explicitly testing network features
📚 Learning: 2025-12-02T01:29:57.585Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T01:29:57.585Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing using in-memory transport instead of HTTP transport, unless explicitly testing network features
Applied to files:
src/fastmcp/utilities/mcp_config.pysrc/fastmcp/client/transports.pysrc/fastmcp/mcp_config.py
🧬 Code graph analysis (5)
src/fastmcp/utilities/mcp_config.py (3)
src/fastmcp/client/transports.py (1)
ClientTransport(75-117)src/fastmcp/mcp_config.py (3)
to_transport(120-124)to_transport(157-165)to_transport(208-230)src/fastmcp/server/proxy.py (1)
ProxyClient(520-618)
src/fastmcp/client/client.py (1)
src/fastmcp/client/transports.py (10)
infer_transport(989-989)infer_transport(993-993)infer_transport(997-997)infer_transport(1001-1001)infer_transport(1004-1004)infer_transport(1008-1008)infer_transport(1012-1014)infer_transport(1018-1022)infer_transport(1026-1026)infer_transport(1029-1125)
src/fastmcp/server/proxy.py (4)
src/fastmcp/server/context.py (2)
fastmcp(152-157)request_context(181-207)src/fastmcp/utilities/types.py (1)
find_kwarg_by_type(152-175)src/fastmcp/server/dependencies.py (1)
get_context(34-40)src/fastmcp/client/client.py (1)
call_tool_mcp(889-932)
src/fastmcp/client/transports.py (1)
src/fastmcp/mcp_config.py (1)
MCPConfig(244-295)
src/fastmcp/mcp_config.py (2)
src/fastmcp/client/client.py (1)
Client(107-1018)src/fastmcp/client/transports.py (1)
ClientTransport(75-117)
⏰ 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: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (7)
src/fastmcp/client/client.py (1)
242-254: LGTM!The client configuration parameters are properly collected and forwarded to
infer_transport. This enables downstream transport and proxy creation to receive client initialization parameters, which aligns with the PR objectives.src/fastmcp/mcp_config.py (1)
92-108: LGTM!The addition of
**client_kwargsto the signature and forwarding to theClientconstructor enables client configuration to propagate through the transforming MCP server pathway, which aligns with the PR objectives.src/fastmcp/client/transports.py (3)
940-964: LGTM!The addition of
**client_kwargstoMCPConfigTransport.__init__and forwarding tomcp_config_to_servers_and_transportsenables client configuration to propagate through the MCPConfig transport pathway.
1003-1004: LGTM!The overload for
MCPConfigwith**client_kwargsprovides proper type hints for the extended signature, ensuring type safety for callers.
1029-1038: LGTM!The addition of
**client_kwargsto theinfer_transportsignature enables client configuration to be forwarded when constructing transports.src/fastmcp/utilities/mcp_config.py (2)
17-25: LGTM!The addition of
**client_kwargstomcp_config_to_servers_and_transportsand forwarding tomcp_server_type_to_servers_and_transportsenables client configuration to propagate through the MCP config pathway.
55-55: LGTM!The forwarding of
**client_kwargstoProxyClientenables client configuration (handlers, callbacks) to be properly initialized in non-transforming server proxies, which aligns with the PR objectives.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Test Failure AnalysisSummary: The tests are failing due to a syntax error in Root Cause: In the changes to # Line 1114 - Incorrect (duplicated with leading spaces)
inferred_transport = MCPConfigTransport(
inferred_transport = MCPConfigTransport(
config=cast(dict | MCPConfig, transport),
**client_kwargs,
)This causes a Suggested Solution:
After fixing these issues, run: uv run prek run --all-files # Fix any linting/formatting issues
uv run pytest # Verify all tests passDetailed AnalysisError Log ExcerptAll test jobs failed with the same error during import: This is a Python syntax error that occurs during module import, preventing any tests from running. Related Files
|
|
MCPTransport is supposed to be a friendly "Helper" -- I think if you need more advanced capabilities then you should consider setting up the servers and transports yourself? |
|
@strawgate I would agree for the second problem which the PR fixes (passing client callbacks to proxies) since it requires significantly more changes to the code and may not be needed by most users. But, for the first issue (passing metadata to proxy tool calls), it only adds minimal code contained to a single method in |
jlowin
left a comment
There was a problem hiding this comment.
@cegersdoerfer I agree with @strawgate here -- passing the kwargs through so many layers does work but IMO is a smell that we're overloading the functionality. However the meta-passing is an excellent enhancement that we should include - if you wouldn't mind removing the kwargs from the PR, we can get this merged!
|
@jlowin done! Let me know if anything else is needed |
Test Failure AnalysisSummary: Two Windows-specific tests are failing due to pytest-xdist worker crashes. These are known flaky tests unrelated to the PR changes. Root Cause: The test failures are caused by a known Windows-specific flakiness in tests that spawn subprocess servers using The failing tests:
Both tests:
This pattern of spawning and killing subprocesses under parallel test execution on Windows triggers race conditions in pytest-xdist worker management. Evidence this is NOT related to the PR:
Suggested Solution: This is a known flaky test issue that should be addressed separately:
For this specific PR: The PR is functionally correct and all non-Windows tests pass. The failures are environmental, not code-related. Detailed AnalysisTest Failure LogsPR Changes (Minimal and Unrelated)The PR makes only two small changes:
Neither change touches:
Known Issue References
Related FilesTest file: No code changes in:
Note: This is an updated analysis for the test failure that occurred after the PR was merged to main (workflow run #19994439585). |
Description
This PR solves 2 issues:
Contributors Checklist
Review Checklist