Conversation
marvin-context-protocol
Bot
added
enhancement
Split the monolithic Client class into focused mixin classes: - ClientToolsMixin (tools_client.py) - ClientPromptsMixin (prompts.py) - ClientResourcesMixin (resources.py) - ClientTaskManagementMixin (task_management.py) Also: - Extract timeout normalization helpers to utilities/timeout.py - Split SSETransport into its own module (transports/sse.py) - Clean up unused imports across client modules - Fix bare except clauses to catch specific McpError Note: ty reports unresolved-attribute errors on mixin classes because it doesn't understand that mixins will be composed with Client. This is a known limitation of mixin typing in Python. The code is functionally correct and all tests pass.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a032c8308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(value, datetime.timedelta): | ||
| return value.total_seconds() | ||
| if isinstance(value, int | float): | ||
| return float(value) |
There was a problem hiding this comment.
normalize_timeout_to_seconds now converts any numeric value to a float, so init_timeout=0 becomes 0.0 instead of None. Client.__init__ uses this helper for the init handshake timeout and documents “Set to 0 to disable”, but with this normalization the subsequent anyio.fail_after(0.0) will immediately time out, breaking clients that rely on 0 as the sentinel. Consider treating 0 (and possibly timedelta(0)) as None for init timeouts.
Useful? React with 👍 / 👎.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughClient is refactored to compose functionality via four new mixins (ClientResourcesMixin, ClientPromptsMixin, ClientToolsMixin, ClientTaskManagementMixin), moving many MCP methods out of the core Client. Timeout handling is centralized with normalize_timeout_to_seconds and normalize_timeout_to_timedelta. Transport code is reorganized: StreamableHttpTransport introduced in the http module and SSETransport moved to a dedicated sse module. New mixins implement SEP-1686 task flows (task creation/registration and graceful degradation), and session lifecycle logic and session-monitoring usage are standardized across MCP call sites. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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)
src/fastmcp/client/transports/http.py (1)
94-100: Fix runtime error when numeric timeout values are passed to Client.The
Clientacceptstimeoutasdatetime.timedelta | float | int | None(line 260), but directly assigns this toread_timeout_secondsinSessionKwargswithout normalization (line 290). The http transport then usescast()(a no-op at runtime) and calls.total_seconds()on what might be a numeric value, causing a TypeError at runtime.The
normalize_timeout_to_timedelta()utility already handles both forms correctly. Apply it in http.py (and the identical pattern in sse.py):Suggested fix
- if session_kwargs.get("read_timeout_seconds") is not None: - read_timeout_seconds = cast( - datetime.timedelta, session_kwargs.get("read_timeout_seconds") - ) - timeout = httpx.Timeout(30.0, read=read_timeout_seconds.total_seconds()) + read_timeout = normalize_timeout_to_timedelta( + session_kwargs.get("read_timeout_seconds") + ) + if read_timeout is not None: + timeout = httpx.Timeout(30.0, read=read_timeout.total_seconds())Apply the same fix to
sse.pylines 72–76.
♻️ Duplicate comments (2)
src/fastmcp/utilities/timeout.py (1)
8-45: Preserve “0 disables timeout” semantics.Both helpers currently convert
0(andtimedelta(0)) into a zero timeout, which can trigger immediate timeouts where0is documented as “disabled”. Consider mapping zero toNone(or add an explicit flag for this behavior) to preserve existing semantics.🔧 Suggested adjustment
def normalize_timeout_to_timedelta( value: int | float | datetime.timedelta | None, ) -> datetime.timedelta | None: @@ - if value is None: - return None - if isinstance(value, datetime.timedelta): - return value - if isinstance(value, int | float): - return datetime.timedelta(seconds=float(value)) + if value is None: + return None + if isinstance(value, datetime.timedelta): + return None if value.total_seconds() == 0 else value + if isinstance(value, int | float): + return None if float(value) == 0.0 else datetime.timedelta(seconds=float(value)) raise TypeError(f"Invalid timeout type: {type(value)}") @@ def normalize_timeout_to_seconds( value: int | float | datetime.timedelta | None, ) -> float | None: @@ - if value is None: - return None - if isinstance(value, datetime.timedelta): - return value.total_seconds() - if isinstance(value, int | float): - return float(value) + if value is None: + return None + if isinstance(value, datetime.timedelta): + return None if value.total_seconds() == 0 else value.total_seconds() + if isinstance(value, int | float): + return None if float(value) == 0.0 else float(value) raise TypeError(f"Invalid timeout type: {type(value)}")src/fastmcp/client/prompts.py (1)
221-289: Propagate trace context for task-mode prompt requests.
_get_prompt_as_taskbypassesinject_trace_context, so trace metadata is dropped in task-mode. This breaks trace correlation for background prompt execution.🩹 Proposed fix
- request = mcp.types.GetPromptRequest( + propagated_meta = inject_trace_context(meta) + request = mcp.types.GetPromptRequest( params=mcp.types.GetPromptRequestParams( name=name, arguments=serialized_arguments, task=mcp.types.TaskMetadata(ttl=ttl), - _meta=meta, # type: ignore[unknown-argument] # pydantic alias + _meta=propagated_meta, # type: ignore[unknown-argument] # pydantic alias ) )
🧹 Nitpick comments (1)
src/fastmcp/client/tools_client.py (1)
357-383: Prefer narrower exception handling and preserve tracebacks while parsing.Catching
Exceptionand logging viaerrorhides stack traces for schema parsing failures. Consider narrowing to validation/parsing errors and usinglogger.exceptionfor diagnosability.🔧 Possible adjustment
- except Exception as e: - logger.error( - f"[{client_name or 'client'}] Error parsing structured content: {e}" - ) + except (ValueError, TypeError) as e: + logger.exception( + f"[{client_name or 'client'}] Error parsing structured content: {e}" + )
| if isinstance(uri, str): | ||
| uri = AnyUrl(uri) # Ensure AnyUrl | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/fastmcp/client/resources.pyRepository: jlowin/fastmcp
Length of output: 14558
🏁 Script executed:
python3 << 'EOF'
from pydantic import AnyUrl
try:
AnyUrl("invalid uri with spaces")
except Exception as e:
print(f"Exception type: {type(e).__name__}")
print(f"Exception module: {type(e).__module__}")
print(f"Exception message: {str(e)}")
EOFRepository: jlowin/fastmcp
Length of output: 144
Handle invalid URI strings consistently across read_resource, read_resource_mcp, and _read_resource_as_task.
read_resource (lines 250-256) wraps the AnyUrl(uri) conversion in try/except to raise ValueError on invalid URIs, but read_resource_mcp (lines 156-157) and _read_resource_as_task (lines 281-282) allow raw AnyUrl validation errors to propagate. For consistent error reporting across all three methods, apply the same error handling pattern to both locations.
🔧 Suggested handling
- if isinstance(uri, str):
- uri = AnyUrl(uri) # Ensure AnyUrl
+ if isinstance(uri, str):
+ try:
+ uri = AnyUrl(uri)
+ except Exception as e:
+ raise ValueError(
+ f"Provided resource URI is invalid: {str(uri)!r}"
+ ) from e
@@
- if isinstance(uri, str):
- uri = AnyUrl(uri)
+ if isinstance(uri, str):
+ try:
+ uri = AnyUrl(uri)
+ except Exception as e:
+ raise ValueError(
+ f"Provided resource URI is invalid: {str(uri)!r}"
+ ) from e| class ClientToolsMixin: | ||
| """Mixin providing tool-related methods for Client.""" | ||
|
|
||
| # --- Tools --- | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "tools_client.py" --type fRepository: jlowin/fastmcp
Length of output: 92
🏁 Script executed:
wc -l src/fastmcp/client/tools_client.pyRepository: jlowin/fastmcp
Length of output: 96
🏁 Script executed:
head -50 src/fastmcp/client/tools_client.py | cat -nRepository: jlowin/fastmcp
Length of output: 2051
🏁 Script executed:
sed -n '32,100p' src/fastmcp/client/tools_client.py | cat -nRepository: jlowin/fastmcp
Length of output: 3053
🏁 Script executed:
fd "client.py" src/fastmcp/client --type fRepository: jlowin/fastmcp
Length of output: 121
🏁 Script executed:
head -100 src/fastmcp/client/client.py | cat -nRepository: jlowin/fastmcp
Length of output: 3395
🏁 Script executed:
grep -n "^class Client" src/fastmcp/client/client.pyRepository: jlowin/fastmcp
Length of output: 104
🏁 Script executed:
sed -n '120,200p' src/fastmcp/client/client.py | cat -nRepository: jlowin/fastmcp
Length of output: 3861
🏁 Script executed:
sed -n '200,300p' src/fastmcp/client/client.py | cat -nRepository: jlowin/fastmcp
Length of output: 4058
🏁 Script executed:
sed -n '300,400p' src/fastmcp/client/client.py | cat -nRepository: jlowin/fastmcp
Length of output: 4895
🏁 Script executed:
grep -n "_await_with_session_monitoring\|_submitted_task_ids" src/fastmcp/client/client.py | head -20Repository: jlowin/fastmcp
Length of output: 408
🏁 Script executed:
sed -n '647,700p' src/fastmcp/client/client.py | cat -nRepository: jlowin/fastmcp
Length of output: 2497
🏁 Script executed:
sed -n '1,31p' src/fastmcp/client/tools_client.py | cat -nRepository: jlowin/fastmcp
Length of output: 1255
🏁 Script executed:
grep -n "_parse_call_tool_result" src/fastmcp/client/tools_client.pyRepository: jlowin/fastmcp
Length of output: 329
🏁 Script executed:
sed -n '332,380p' src/fastmcp/client/tools_client.py | cat -nRepository: jlowin/fastmcp
Length of output: 2439
🏁 Script executed:
sed -n '332,391p' src/fastmcp/client/tools_client.py | cat -nRepository: jlowin/fastmcp
Length of output: 2838
Add type annotations to ClientToolsMixin to resolve type-checker errors.
The mixin uses attributes (session, transport, name, _await_with_session_monitoring, _submitted_task_ids) that exist on the concrete Client class but are not declared in the mixin itself. Type checkers cannot resolve these. Add a TYPE_CHECKING block with a Protocol to satisfy static analysis without runtime overhead, aligning with the coding guideline requirement for full type annotations.
Additionally, narrow the exception handling at line 49 from bare Exception to a specific exception type per coding guidelines.
Split the monolithic Client class into domain-specific mixins: - ClientToolsMixin: tool listing and calling - ClientPromptsMixin: prompt listing and retrieval - ClientResourcesMixin: resource and template operations - ClientTaskManagementMixin: background task management Also extracts timeout normalization to utilities/timeout.py and fixes missing trace context propagation in _*_as_task methods.
1 participant
The Client class was getting unwieldy. This splits it into focused mixin classes while keeping the public API unchanged:
Also extracts
normalize_timeout_to_timedeltaandnormalize_timeout_to_secondstoutilities/timeout.py, consolidating duplicated timeout conversion logic from client.py, tools_client.py, and the HTTP transports.