Fix keep_alive passthrough in StdioMCPServer.to_transport()#2791
Conversation
WalkthroughA new optional field 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization 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). (4)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastmcp/server/middleware/error_handling.py (1)
81-112: Potential bug: wrappedMcpErrorcause falls through to generic internal error.When
error.__cause__is anMcpError, theisinstancecheck on line 83 passes only for the wrapper exception, not the cause. SinceMcpErrorisn't in any of the explicit type checks (lines 92-108), it falls through to the generic "Internal error" on line 110, discarding the originalMcpError's code and message.Additionally, there's an inconsistency:
error_typeis derived from the cause, but all error messages useerror!s(the wrapper's string). This could produce confusing responses where the error code implies one issue (e.g., "Invalid params") but the message describes something else.Consider either:
- Checking if
error.__cause__is anMcpErrorand returning it directly, or- Using consistent sources for both type and message.
🔎 Suggested fix to handle McpError causes
def _transform_error(self, error: Exception) -> Exception: """Transform non-MCP errors to proper MCP errors.""" if isinstance(error, McpError): return error + # If the cause is an McpError, return it directly + if isinstance(error.__cause__, McpError): + return error.__cause__ + if not self.transform_errors: return error # Map common exceptions to appropriate MCP error codes error_type = type(error.__cause__) if error.__cause__ else type(error) + # Use cause's message for consistency when using cause's type + error_source = error.__cause__ if error.__cause__ else errorThen replace
{error!s}with{error_source!s}in the error messages below.
🧹 Nitpick comments (6)
docs/getting-started/installation.mdx (1)
26-28: Consider if duplicate warnings are necessary.This warning about FastMCP 3.0 development is clear and actionable, which is good. However, according to the AI summary, the same warning appears in
docs/getting-started/welcome.mdx(twice in that file).Having the same warning on multiple onboarding pages may be excessive and could create maintenance burden if the message needs to be updated. Consider consolidating these warnings or ensuring they serve different purposes in each location.
As per coding guidelines: MDX documentation should avoid redundancy and maintain consistency across pages.
docs/docs.json (1)
15-15: Time-sensitive banner content will need updating.The banner now promotes the PyAI Conf on March 10th. After this date, the banner will be outdated. Consider creating a follow-up task to update or remove this banner after the event.
src/fastmcp/server/auth/auth.py (1)
307-332: Consider applying the set_mcp_path pattern consistently across provider types.
OAuthProvider.get_routes()now callsset_mcp_path()and uses the storedself._resource_url(line 481), whileRemoteAuthProvider.get_routes()still computes the resource URL locally withself._get_resource_url(mcp_path)(line 318).This inconsistency isn't a bug, but applying the pattern uniformly would improve maintainability.
🔎 Optional refactor to align RemoteAuthProvider with the new pattern
def get_routes( self, mcp_path: str | None = None, ) -> list[Route]: """Get routes for this provider. Creates protected resource metadata routes (RFC 9728). """ + # Configure resource URL before creating routes + self.set_mcp_path(mcp_path) + routes = [] - # Get the resource URL based on the MCP path - resource_url = self._get_resource_url(mcp_path) - - if resource_url: + if self._resource_url: # Add protected resource metadata routes routes.extend( create_protected_resource_routes( - resource_url=resource_url, + resource_url=self._resource_url, authorization_servers=self.authorization_servers, scopes_supported=self.token_verifier.required_scopes, resource_name=self.resource_name, resource_documentation=self.resource_documentation, ) ) return routesAlso applies to: 426-427, 473-485
docs/integrations/supabase.mdx (1)
40-42: Clarify when auth_route parameter is needed.The inline comment "if self-hosting and using custom routes" suggests this parameter is conditional, but the code example shows it as part of the standard configuration. Consider either:
- Moving this parameter to a separate example specifically for self-hosted scenarios, or
- Showing it as an optional parameter with a comment that it defaults to "/auth/v1"
This would help users understand they can omit this parameter for standard Supabase Cloud deployments.
🔎 Suggested documentation structure
# Configure Supabase Auth auth = SupabaseProvider( project_url="https://abc123.supabase.co", base_url="http://localhost:8000", - auth_route="/my/auth/route" # if self-hosting and using custom routes )Then add a separate note or example:
**For self-hosted Supabase with custom auth routes:** If you're self-hosting Supabase Auth with a custom route, specify it: \```python auth = SupabaseProvider( project_url="https://your-instance.com", base_url="http://localhost:8000", auth_route="/my/auth/route" # defaults to "/auth/v1" ) \```src/fastmcp/prompts/prompt_manager.py (1)
110-111: LGTM!The FastMCPError propagation is implemented correctly and aligns with the error handling pattern in dependencies.py. This ensures proper error categorization throughout the call stack.
Consider adding a clarifying comment similar to dependencies.py for consistency:
except FastMCPError: + # Let FastMCPError subclasses (ToolError, ResourceError, etc.) + # propagate unchanged so they can be handled appropriately raisesrc/fastmcp/server/server.py (1)
479-480: Consider adding a timeout for robustness.The simplified cleanup with
suppress(asyncio.CancelledError)is cleaner than a timeout-based approach. However, if the worker task doesn't respect cancellation (e.g., due to a bug or blocking operation), this will block indefinitely during shutdown.Consider using
asyncio.wait_for()with a generous timeout as a safety net:🔎 Proposed enhancement
finally: worker_task.cancel() - with suppress(asyncio.CancelledError): - await worker_task + try: + with suppress(asyncio.CancelledError): + await asyncio.wait_for(worker_task, timeout=30.0) + except asyncio.TimeoutError: + logger.warning( + f"Worker task did not stop within 30s after cancellation" + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
README.mdis excluded by none and included by nonepyproject.tomlis excluded by none and included by nonetests/client/test_elicitation.pyis excluded by none and included by nonetests/client/test_sse.pyis excluded by none and included by nonetests/client/test_streamable_http.pyis excluded by none and included by nonetests/client/transports/test_uv_transport.pyis excluded by none and included by nonetests/server/auth/providers/test_supabase.pyis excluded by none and included by nonetests/server/auth/test_oauth_proxy.pyis excluded by none and included by nonetests/server/middleware/test_error_handling.pyis excluded by none and included by nonetests/server/openapi/test_comprehensive.pyis excluded by none and included by nonetests/server/tasks/test_server_tasks_parameter.pyis excluded by none and included by nonetests/server/tasks/test_task_methods.pyis excluded by none and included by nonetests/server/test_dependencies.pyis excluded by none and included by nonetests/server/test_logging.pyis excluded by none and included by nonetests/test_mcp_config.pyis excluded by none and included by nonetests/tools/test_tool.pyis excluded by none and included by nonetests/utilities/test_json_schema.pyis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (23)
docs/changelog.mdxdocs/docs.jsondocs/getting-started/installation.mdxdocs/getting-started/welcome.mdxdocs/integrations/supabase.mdxdocs/updates.mdxexamples/auth/github_oauth/client.pysrc/fastmcp/mcp_config.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/resources/resource_manager.pysrc/fastmcp/server/auth/auth.pysrc/fastmcp/server/auth/oauth_proxy.pysrc/fastmcp/server/auth/providers/supabase.pysrc/fastmcp/server/dependencies.pysrc/fastmcp/server/elicitation.pysrc/fastmcp/server/middleware/error_handling.pysrc/fastmcp/server/openapi/components.pysrc/fastmcp/server/server.pysrc/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_manager.pysrc/fastmcp/utilities/cli.pysrc/fastmcp/utilities/json_schema.pysrc/fastmcp/utilities/openapi/schemas.py
🧰 Additional context used
📓 Path-based instructions (4)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/server/dependencies.pysrc/fastmcp/server/server.pysrc/fastmcp/utilities/json_schema.pysrc/fastmcp/mcp_config.pysrc/fastmcp/server/middleware/error_handling.pysrc/fastmcp/server/auth/providers/supabase.pysrc/fastmcp/server/elicitation.pysrc/fastmcp/server/auth/auth.pysrc/fastmcp/utilities/openapi/schemas.pysrc/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_manager.pysrc/fastmcp/utilities/cli.pysrc/fastmcp/server/openapi/components.pysrc/fastmcp/server/auth/oauth_proxy.py
docs/**/*.{md,mdx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation uses Mintlify framework. Files must be in docs.json to be included. Never modify docs/python-sdk/** (auto-generated)
Files:
docs/docs.jsondocs/getting-started/welcome.mdxdocs/updates.mdxdocs/getting-started/installation.mdxdocs/changelog.mdxdocs/integrations/supabase.mdx
docs/**/*.mdx
📄 CodeRabbit inference engine (docs/.cursor/rules/mintlify.mdc)
docs/**/*.mdx: Use clear, direct language appropriate for technical audiences
Write in second person ('you') for instructions and procedures in MDX documentation
Use active voice over passive voice in MDX technical documentation
Employ present tense for current states and future tense for outcomes in MDX documentation
Maintain consistent terminology throughout all MDX documentation
Keep sentences concise while providing necessary context in MDX documentation
Use parallel structure in lists, headings, and procedures in MDX documentation
Lead with the most important information using inverted pyramid structure in MDX documentation
Use progressive disclosure in MDX documentation: present basic concepts before advanced ones
Break complex procedures into numbered steps in MDX documentation
Include prerequisites and context before instructions in MDX documentation
Provide expected outcomes for each major step in MDX documentation
End sections with next steps or related information in MDX documentation
Use descriptive, keyword-rich headings for navigation and SEO in MDX documentation
Focus on user goals and outcomes rather than system features in MDX documentation
Anticipate common questions and address them proactively in MDX documentation
Include troubleshooting for likely failure points in MDX documentation
Provide multiple pathways (beginner vs advanced) but offer an opinionated path to avoid overwhelming users in MDX documentation
Always include complete, runnable code examples that users can copy and execute in MDX documentation
Show proper error handling and edge case management in MDX code examples
Use realistic data instead of placeholder values in MDX code examples
Include expected outputs and results for verification in MDX code examples
Test all code examples thoroughly before publishing in MDX documentation
Specify language and include filename when relevant in MDX code examples
Add explanatory comments for complex logic in MDX code examples
Document all API...
Files:
docs/getting-started/welcome.mdxdocs/updates.mdxdocs/getting-started/installation.mdxdocs/changelog.mdxdocs/integrations/supabase.mdx
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.{md,mdx}: Code examples in documentation must explain before showing code and make blocks fully runnable (include imports)
Documentation structure: Headers form navigation guide with logical H2/H3 hierarchy. Content should be user-focused with sections motivating features (why) before mechanics (how). Use prose over code comments for important information
Never use 'This isn't...' or 'not just...' constructions in writing - state what something IS directly. Avoid defensive writing patterns
Files:
docs/getting-started/welcome.mdxdocs/updates.mdxdocs/getting-started/installation.mdxdocs/changelog.mdxdocs/integrations/supabase.mdx
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR #2505 in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/server/dependencies.pydocs/getting-started/welcome.mdxsrc/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/server/dependencies.pydocs/getting-started/welcome.mdxdocs/getting-started/installation.mdxsrc/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_manager.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Never use bare except - be specific with exception types
Applied to files:
src/fastmcp/server/dependencies.pysrc/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Follow existing patterns and maintain consistency in code implementation
Applied to files:
src/fastmcp/server/dependencies.pydocs/getting-started/welcome.mdxsrc/fastmcp/resources/resource_manager.pysrc/fastmcp/prompts/prompt_manager.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/server/dependencies.pydocs/getting-started/welcome.mdxexamples/auth/github_oauth/client.py
📚 Learning: 2025-11-26T21:52:08.947Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: docs/.cursor/rules/mintlify.mdc:0-0
Timestamp: 2025-11-26T21:52:08.947Z
Learning: Applies to docs/**/*.mdx : Add appropriate warnings for destructive or security-sensitive actions in MDX documentation
Applied to files:
docs/getting-started/welcome.mdxdocs/getting-started/installation.mdx
📚 Learning: 2025-11-03T17:36:13.363Z
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 2355
File: docs/clients/client.mdx:226-246
Timestamp: 2025-11-03T17:36:13.363Z
Learning: In FastMCP documentation, prefer showing the happy path in onboarding examples without over-explaining edge cases or adding defensive checks, as this reduces cognitive burden for new users learning the API.
Applied to files:
docs/getting-started/welcome.mdx
📚 Learning: 2025-11-26T21:52:08.947Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: docs/.cursor/rules/mintlify.mdc:0-0
Timestamp: 2025-11-26T21:52:08.947Z
Learning: Applies to docs/**/*.mdx : Include prerequisites and context before instructions in MDX documentation
Applied to files:
docs/getting-started/welcome.mdx
📚 Learning: 2025-11-26T21:52:08.947Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: docs/.cursor/rules/mintlify.mdc:0-0
Timestamp: 2025-11-26T21:52:08.947Z
Learning: Applies to docs/**/*.mdx : Explain prerequisites clearly before beginning instructions in MDX documentation
Applied to files:
docs/getting-started/welcome.mdx
📚 Learning: 2025-11-26T21:52:08.947Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: docs/.cursor/rules/mintlify.mdc:0-0
Timestamp: 2025-11-26T21:52:08.947Z
Learning: Applies to docs/**/*.mdx : Lead with the most important information using inverted pyramid structure in MDX documentation
Applied to files:
docs/getting-started/welcome.mdx
📚 Learning: 2025-12-01T15:48:05.095Z
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR #2505 in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.
Applied to files:
docs/updates.mdxdocs/changelog.mdxsrc/fastmcp/server/auth/oauth_proxy.py
📚 Learning: 2025-11-26T21:51:44.174Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Note that Resources and Resource Templates are distinct objects but both handled by ResourceManager, requiring coordinated updates when changes affect either object type
Applied to files:
src/fastmcp/resources/resource_manager.py
📚 Learning: 2025-11-26T21:51:44.174Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
Applied to files:
src/fastmcp/prompts/prompt_manager.pysrc/fastmcp/tools/tool_manager.py
🧬 Code graph analysis (8)
src/fastmcp/server/dependencies.py (1)
src/fastmcp/exceptions.py (1)
FastMCPError(6-7)
src/fastmcp/server/auth/providers/supabase.py (1)
src/fastmcp/server/auth/providers/jwt.py (1)
JWTVerifier(165-498)
src/fastmcp/server/auth/auth.py (1)
src/fastmcp/server/auth/oauth_proxy.py (1)
set_mcp_path(905-926)
src/fastmcp/resources/resource_manager.py (2)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/exceptions.py (3)
FastMCPError(6-7)NotFoundError(34-35)ResourceError(14-15)
src/fastmcp/prompts/prompt_manager.py (2)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/exceptions.py (3)
FastMCPError(6-7)NotFoundError(34-35)PromptError(22-23)
src/fastmcp/tools/tool.py (1)
src/fastmcp/utilities/json_schema.py (2)
compress_schema(240-270)resolve_root_ref(7-43)
src/fastmcp/tools/tool_manager.py (1)
src/fastmcp/exceptions.py (4)
FastMCPError(6-7)NotFoundError(34-35)ToolError(18-19)ValidationError(10-11)
src/fastmcp/server/auth/oauth_proxy.py (1)
src/fastmcp/server/auth/jwt_issuer.py (4)
JWTIssuer(74-236)issue_access_token(100-145)issue_refresh_token(147-193)verify_token(195-236)
🪛 Ruff (0.14.10)
src/fastmcp/server/auth/oauth_proxy.py
936-939: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (25)
src/fastmcp/mcp_config.py (2)
146-148: LGTM! Field addition looks correct.The
keep_alivefield is properly typed, documented, and logically placed with other execution context fields. The implementation follows existing patterns in the class.
167-167: The keep_alive passthrough is correctly implemented.
StdioTransport.__init__acceptskeep_alive: bool | None = None, so the parameter is properly defined to handle both values andNonedefaults. The passthrough at line 167 is correct and will fix the bug as described in the PR.examples/auth/github_oauth/client.py (1)
13-13: Verify this change aligns with PR objectives.This PR claims to fix
keep_alivepassthrough inStdioMCPServer.to_transport(), but this file only changes the server URL from127.0.0.1tolocalhost. This appears unrelated to the stated PR objective.Additionally, while
localhostand127.0.0.1typically resolve to the same address, there can be subtle differences:
localhostmay resolve to IPv6::1on some systems127.0.0.1explicitly uses IPv4- This could cause connection issues on IPv6-only or misconfigured systems
Was this file change intended for this PR, or should it be in a separate PR?
docs/docs.json (1)
44-44: Verify the website URL change is intentional.The footer website URL changed from
https://prefect.aitohttps://www.prefect.io. Please confirm this change is correct and that the.iodomain is now the official/canonical URL.docs/changelog.mdx (1)
7-2227: LGTM! Comprehensive changelog additions.The changelog entries are well-structured, comprehensive, and follow a consistent format. The release notes provide clear categorization of changes (New Features, Enhancements, Fixes, Docs, etc.) and include contributor recognition.
src/fastmcp/server/elicitation.py (1)
297-319: LGTM! Enum schema structure correctly distinguishes single vs multi-select.The implementation properly generates:
- Single-select:
{"type": "string", "oneOf": [...]}to indicate a string primitive- Multi-select:
{"anyOf": [...]}without type (used as array items)This aligns with MCP elicitation schema requirements and SEP-1330.
docs/updates.mdx (1)
8-78: Documentation additions look good.The release notes are clear, well-structured, and follow the existing format. No concerns.
src/fastmcp/server/auth/auth.py (1)
133-145: Clear implementation of MCP path configuration hook.The
set_mcp_pathmethod correctly stores the MCP path and precomputes the resource URL. This enables subclasses like OAuthProxy to perform additional initialization that depends on the MCP endpoint path.src/fastmcp/server/auth/oauth_proxy.py (4)
807-810: Deferred JWTIssuer initialization correctly binds tokens to the resource URL.The JWT issuer is now created in
set_mcp_path()with the audience set to the actual resource URL (e.g.,http://localhost:8000/mcp), ensuring tokens are bound to the specific MCP endpoint. The property pattern with RuntimeError provides a clear failure mode if accessed before initialization.Since
get_routes()is called during server initialization (before any token operations), the initialization order is safe.Also applies to: 905-940
814-816: Lazy import avoids unnecessary sqlite3 dependency.Moving the
DiskStoreimport inside__init__prevents loading sqlite3 when OAuthProxy isn't used. This is good practice for optional dependencies.
1051-1065: Resource URL validation prevents token misuse across servers.The security check ensures the client's requested resource matches the server's resource URL, preventing tokens intended for one server from being used on another. This addresses the "confused deputy" problem in OAuth flows.
1224-1241: Enhanced logging improves token lifetime observability.The extensive TTL and IdP response logging will help diagnose token expiration issues and upstream provider integration problems.
Also applies to: 1482-1496, 1648-1651, 1894-1898
src/fastmcp/server/auth/providers/supabase.py (1)
136-137: Configurable auth_route enables self-hosted Supabase Auth flexibility.The auth_route parameter correctly propagates through JWKS URI, issuer, authorization servers, and metadata endpoints. This allows users with custom Supabase Auth deployments to configure non-standard paths.
Also applies to: 145-145, 170-170
docs/integrations/supabase.mdx (1)
121-123: LGTM!The environment variable documentation is clear and follows the established pattern for other Supabase configuration options.
src/fastmcp/server/dependencies.py (1)
192-195: LGTM!The FastMCPError propagation logic is correct and aligns with the error handling pattern established across the codebase. This ensures that domain-specific errors (ToolError, ResourceError, PromptError) propagate unchanged for proper categorization and handling.
src/fastmcp/utilities/openapi/schemas.py (1)
542-549: No issue identified. The converter already implements proper version-specific handling: nullable field conversion is explicitly gated to OpenAPI 3.0 only (line 90 of json_schema_converter.py), while OpenAPI 3.1+ schemas skip this conversion and receive only necessary cleanup (field removal, oneOf→anyOf conversion). This aligns with the OpenAPI 3.1 specification, which is based on JSON Schema 2020-12 and does not use the nullable field. Thestartswith("3")check in schemas.py correctly delegates version-specific logic to the converter function.Likely an incorrect or invalid review comment.
src/fastmcp/tools/tool.py (1)
35-35: LGTM! Correct implementation of MCP spec requirement.The addition of
resolve_root_refproperly addresses the MCP specification requirement for root-leveltype: "object"in output schemas. The placement aftercompress_schemaand before returning ensures that self-referential Pydantic models (which generate$refat root level) are correctly resolved while preserving$defsfor nested references.Also applies to: 562-564
src/fastmcp/tools/tool_manager.py (1)
11-11: LGTM! Consistent error propagation pattern.The explicit handling of
FastMCPErrorandValidationErrorensures these exceptions propagate unchanged while other exceptions are wrapped inToolErrorwith configurable detail masking. This aligns with the error handling updates across other manager classes in this PR.Also applies to: 161-164
src/fastmcp/utilities/json_schema.py (1)
7-43: LGTM! Well-implemented utility function.The
resolve_root_reffunction correctly addresses the MCP spec requirement by inlining root-level$refdefinitions while preserving$defsfor nested references. The implementation is:
- Properly typed with full annotations (Python ≥ 3.10)
- Well-documented with clear docstring and example
- Idempotent and safe (returns original if no resolution needed)
- Correctly scoped to only handle local
#/$defs/referencessrc/fastmcp/utilities/cli.py (1)
200-254: Verify removal of connection details is intentional.The function signature was simplified to only accept
server: FastMCP[Any], removing thetransport,host,port, andpathparameters. The banner now displays only the server name and marketing URLs (docs, deploy) without showing connection details like host/port.If users need to know where the server is listening (especially for HTTP transport), consider whether this information should still be displayed or logged elsewhere.
The v3 notice panel is a nice touch for communicating the upcoming major version to users.
src/fastmcp/resources/resource_manager.py (1)
13-13: LGTM! Consistent FastMCPError propagation.The exception handling updates ensure
FastMCPErrorinstances propagate unchanged through resource operations while other exceptions are wrapped with configurable detail masking. This maintains consistency with the error handling pattern established intool_manager.py.Also applies to: 271-273, 301-303, 323-324
src/fastmcp/server/server.py (4)
1-3100: PR objective mismatch: Changes unrelated to stated bugfix.The PR title states "Fix keep_alive passthrough in StdioMCPServer.to_transport()", but this file contains only refactorings related to Docket registration naming and lifecycle cleanup. There are no changes to StdioMCPServer, to_transport(), or keep_alive parameters in this file.
This makes the PR harder to review and understand. Consider either:
- Separating these refactorings into a different PR
- Updating the PR description to mention these additional changes
- Confirming that the keep_alive fix is in a different file not included in this review
423-448: LGTM: Explicit task names improve clarity.The explicit
names=[...]parameters in docket registrations make task naming predictable and align with how components are accessed (tools/prompts by key, resources/templates by name).
521-551: LGTM: Consistent explicit naming for mounted servers.The explicit task names for mounted server components correctly apply prefix and tool_names override logic, maintaining consistency with how these components are accessed.
2499-2499: LGTM: Simplified banner logging.The simplified
log_server_banner(server=self)calls reduce coupling by eliminating explicit parameter passing, letting the banner function extract needed information directly from the server.Also applies to: 2564-2564
| <Warning> | ||
| **FastMCP 3.0** is in development and may include breaking changes. To avoid unexpected issues, pin your dependency to v2: `fastmcp<3` | ||
| </Warning> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate warning blocks.
According to the AI summary, this warning about FastMCP 3.0 appears twice in this file. Additionally, the same warning is in installation.mdx. Having the same warning multiple times in a single page provides a poor user experience and creates unnecessary maintenance burden.
Recommendation: Keep at most one instance of this warning per page, and consider if it's needed on both the welcome and installation pages.
As per coding guidelines: Avoid redundancy in MDX documentation and maintain concise, scannable content.
|
|
||
| self.project_url = str(settings.project_url).rstrip("/") | ||
| self.base_url = AnyHttpUrl(str(settings.base_url).rstrip("/")) | ||
| self.auth_route = settings.auth_route.strip("/") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate that auth_route is not empty after stripping.
If auth_route is provided as "/" or whitespace, strip("/") will result in an empty string, leading to malformed URLs like {project_url}//.well-known/....
🔎 Suggested validation fix
- self.auth_route = settings.auth_route.strip("/")
+ self.auth_route = settings.auth_route.strip("/")
+ if not self.auth_route:
+ raise ValueError("auth_route cannot be empty after removing slashes")Alternatively, add a validator to SupabaseProviderSettings:
@field_validator("auth_route", mode="after")
@classmethod
def _validate_auth_route(cls, v: str) -> str:
stripped = v.strip("/")
if not stripped:
raise ValueError("auth_route cannot be empty after removing slashes")
return stripped| str(self._client.base_url) if hasattr(self._client, "base_url") else "" | ||
| ) or "http://localhost" |
There was a problem hiding this comment.
Potential bug: Handle None base_url explicitly.
If self._client.base_url is None, the expression str(self._client.base_url) evaluates to the string "None", which is truthy and will not trigger the fallback to "http://localhost". This would result in an invalid base URL being used for requests.
According to httpx documentation, AsyncClient.base_url is Optional[URL], so this is a realistic scenario.
🔎 Proposed fix
try:
# Get base URL from client
base_url = (
- str(self._client.base_url) if hasattr(self._client, "base_url") else ""
+ str(self._client.base_url)
+ if hasattr(self._client, "base_url") and self._client.base_url
+ else ""
) or "http://localhost"Or more explicitly:
try:
# Get base URL from client
- base_url = (
- str(self._client.base_url) if hasattr(self._client, "base_url") else ""
- ) or "http://localhost"
+ if hasattr(self._client, "base_url") and self._client.base_url:
+ base_url = str(self._client.base_url)
+ else:
+ base_url = "http://localhost"📝 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.
| str(self._client.base_url) if hasattr(self._client, "base_url") else "" | |
| ) or "http://localhost" | |
| str(self._client.base_url) | |
| if hasattr(self._client, "base_url") and self._client.base_url | |
| else "" | |
| ) or "http://localhost" |
eb16de6 to
e63d670
Compare
Fixes a bug where
StdioMCPServer.to_transport()was not passing through thekeep_aliveparameter to the createdStdioTransport, causing subprocesses to persist after the Client context manager exits even whenkeep_alive=Falsewas specified.Changes
keep_alive: bool | None = Nonefield toStdioMCPServerclassto_transport()method to passkeep_alive=self.keep_alivewhen creatingStdioTransportExample
Previously,
transport.keep_alivewould always beTrueregardless of the value set onStdioMCPServer.