Fix type errors for ty 0.0.1-alpha.31 upgrade#2561
Conversation
Add type ignores and fixes for ty's stricter checking: - Path(None) guards in cli.py - isinstance checks for ElicitRequestFormParams (URL elicitation support) - TODO(ty) comments for match/isinstance narrowing bugs - Method override type ignores for generic covariance - Starlette Middleware typing workarounds - Dynamic type construction ignores in json_schema_type.py
- Add asserts for optional attribute access in tests - Add type ignores for dynamic httpx transport internals - Add TODO(ty) comments for `in` operator on str|bytes - Add TODO(ty) comments for Starlette Middleware typing - Use cast for prompt.fn async validation in server.py
marvin-context-protocol
Bot
added
the
enhancement
Fixes additional test file type errors discovered after upgrade.
WalkthroughThis pull request adds runtime guards and handling for distinct request parameter types, while introducing widespread type-checking suppression annotations throughout the codebase. Key functional changes include enforcing None-checks in the CLI's prepare command, handling both form-based and URL-based ElicitRequest types in the client elicitation flow, and extending the proxy server's elicitation handling. The majority of changes consist of adding Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
src/fastmcp/prompts/prompt.py (1)
208-209: Consider adding a TODO comment for consistency.The
type: ignore[assignment]is appropriate here since the type checker doesn't properly narrowstaticmethod.__func__after theisinstancecheck. However, for consistency with other type suppressions in this PR (see lines 120-121 inhttp.py, line 378 intransports.py), consider adding a TODO comment:if isinstance(fn, staticmethod): - fn = fn.__func__ # type: ignore[assignment] + # TODO(ty): remove when ty supports staticmethod unwrapping + fn = fn.__func__ # type: ignore[assignment]src/fastmcp/server/proxy.py (1)
639-643: Consider adding parameter type annotations for context manager protocol.While the
type: ignore[override]annotation suppresses ty's warning, the__aexit__method parameters should ideally be typed according to the context manager protocol:async def __aexit__( self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: types.TracebackType | None ) -> None:This would eliminate the need for the type ignore and improve type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/client/test_client.pyis excluded by none and included by nonetests/client/transports/test_transports.pyis excluded by none and included by nonetests/resources/test_resource_template.pyis excluded by none and included by nonetests/server/auth/providers/test_azure.pyis excluded by none and included by nonetests/server/http/test_http_middleware.pyis excluded by none and included by nonetests/server/test_server_docket.pyis excluded by none and included by nonetests/server/test_server_lifespan.pyis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (20)
src/fastmcp/cli/cli.py(2 hunks)src/fastmcp/client/elicitation.py(3 hunks)src/fastmcp/client/messages.py(1 hunks)src/fastmcp/client/roots.py(1 hunks)src/fastmcp/client/tasks.py(1 hunks)src/fastmcp/client/transports.py(1 hunks)src/fastmcp/experimental/sampling/handlers/openai.py(1 hunks)src/fastmcp/prompts/prompt.py(1 hunks)src/fastmcp/server/auth/auth.py(1 hunks)src/fastmcp/server/auth/oauth_proxy.py(3 hunks)src/fastmcp/server/auth/providers/in_memory.py(2 hunks)src/fastmcp/server/auth/providers/oci.py(2 hunks)src/fastmcp/server/context.py(2 hunks)src/fastmcp/server/elicitation.py(1 hunks)src/fastmcp/server/http.py(1 hunks)src/fastmcp/server/proxy.py(6 hunks)src/fastmcp/server/server.py(1 hunks)src/fastmcp/utilities/components.py(3 hunks)src/fastmcp/utilities/json_schema_type.py(3 hunks)src/fastmcp/utilities/mcp_server_config/v1/mcp_server_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Python source code must use Python ≥3.10 with full type annotations
Never use bareexcept- be specific with exception types
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code organization and style
Files:
src/fastmcp/server/http.pysrc/fastmcp/server/elicitation.pysrc/fastmcp/server/server.pysrc/fastmcp/server/auth/providers/oci.pysrc/fastmcp/server/auth/providers/in_memory.pysrc/fastmcp/client/tasks.pysrc/fastmcp/utilities/json_schema_type.pysrc/fastmcp/client/roots.pysrc/fastmcp/server/auth/auth.pysrc/fastmcp/client/transports.pysrc/fastmcp/experimental/sampling/handlers/openai.pysrc/fastmcp/server/context.pysrc/fastmcp/utilities/components.pysrc/fastmcp/server/proxy.pysrc/fastmcp/cli/cli.pysrc/fastmcp/prompts/prompt.pysrc/fastmcp/client/messages.pysrc/fastmcp/utilities/mcp_server_config/v1/mcp_server_config.pysrc/fastmcp/client/elicitation.pysrc/fastmcp/server/auth/oauth_proxy.py
🧠 Learnings (2)
📚 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:
src/fastmcp/server/auth/providers/in_memory.py
📚 Learning: 2025-12-04T00:17:41.238Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T00:17:41.238Z
Learning: Applies to tests/**/*.py : Use `# type: ignore[attr-defined]` in tests for MCP results instead of type assertions
Applied to files:
src/fastmcp/utilities/components.py
🧬 Code graph analysis (7)
src/fastmcp/server/http.py (1)
src/fastmcp/server/middleware/middleware.py (1)
Middleware(80-208)
src/fastmcp/server/auth/providers/in_memory.py (2)
src/fastmcp/server/auth/providers/jwt.py (3)
load_access_token(367-483)verify_token(485-498)verify_token(535-564)src/fastmcp/server/auth/auth.py (5)
AccessToken(43-46)verify_token(118-129)verify_token(241-243)verify_token(287-289)verify_token(382-395)
src/fastmcp/utilities/components.py (1)
src/fastmcp/server/middleware/middleware.py (1)
copy(63-64)
src/fastmcp/server/proxy.py (2)
src/fastmcp/resources/template.py (1)
from_mcp_template(207-217)src/fastmcp/server/context.py (5)
session(395-405)elicit(573-579)elicit(585-589)elicit(595-599)elicit(604-693)
src/fastmcp/client/messages.py (1)
src/fastmcp/server/dependencies.py (1)
message(400-401)
src/fastmcp/client/elicitation.py (1)
src/fastmcp/utilities/json_schema_type.py (1)
json_schema_to_type(110-190)
src/fastmcp/server/auth/oauth_proxy.py (2)
src/fastmcp/server/auth/providers/in_memory.py (1)
load_access_token(287-296)src/fastmcp/server/auth/providers/jwt.py (1)
load_access_token(367-483)
🪛 Ruff (0.14.7)
src/fastmcp/utilities/json_schema_type.py
268-268: Unused noqa directive (non-enabled: UP007)
Remove unused noqa directive
(RUF100)
⏰ 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)
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
🔇 Additional comments (25)
src/fastmcp/server/http.py (1)
120-121: Well-documented typing workaround.The
type: ignore[arg-type]with accompanying TODO comment is appropriate for handling Starlette's Middleware typing issues. This aligns with the PR's stated objective of working around third-party stub issues.src/fastmcp/experimental/sampling/handlers/openai.py (1)
162-168: Type suppression is safe due to runtime validation.The
type: ignore[assignment]is appropriate here. The runtime check on line 166 (if model_option in get_args(ChatModel)) ensures thatmodel_optionis a validChatModelvalue before assignment, making this type-safe despite the checker's inability to narrow the type.src/fastmcp/server/auth/auth.py (1)
192-199: Consistent typing workaround for Starlette middleware.The
type: ignore[arg-type]annotations with TODO comment are consistent with the same workaround applied insrc/fastmcp/server/http.py(lines 120-121). This appropriately handles the Starlette Middleware typing issue mentioned in the PR objectives.src/fastmcp/server/auth/providers/in_memory.py (2)
287-287: Type suppression for override is appropriate.The
type: ignore[override]is necessary here because the method returnsfastmcp.server.auth.auth.AccessToken(which extends the MCP SDK'sAccessTokenwith additionalclaimsfield) rather than the base SDK type. This is a safe covariant override, but the type checker may not properly recognize it. Runtime behavior is correct.
298-311: Type suppression for override is appropriate.The
type: ignore[override]is consistent with the same issue inload_access_tokenat line 287. The method correctly implements theTokenVerifierprotocol by delegating toload_access_token.src/fastmcp/server/auth/providers/oci.py (1)
174-190: Type suppression appropriate for Pydantic settings pattern.The explicit
dict[str, object]annotation andtype: ignore[arg-type]are appropriate for this Pydantic settings initialization pattern. The dictionary contains mixed types that will be validated at runtime by Pydantic's validation logic (lines 192-210 show the validation checks). This is a standard pattern when passing dynamic kwargs to Pydantic models.src/fastmcp/client/transports.py (1)
378-379: Well-documented workaround for ty limitation.The
type: ignore[arg-type]with TODO comment appropriately handles theUnpack[TypedDict]inference issue explicitly mentioned in the PR objectives. Thesession_kwargsparameter is properly typed at the function signature (line 363), and runtime behavior is unaffected.src/fastmcp/client/roots.py (1)
36-38: Appropriate workaround for isinstance narrowing limitation.The
type: ignore[arg-type]with TODO comment correctly handles the type narrowing issue mentioned in the PR objectives ("isinstance union narrowing adjustments in roots.py"). Theisinstance(handler, list)check on line 36 logically ensureshandleris aRootsList, but ty doesn't properly narrow the union type. Runtime behavior is correct.src/fastmcp/server/server.py (1)
409-412: Castingprompt.fnto async callable is appropriate hereGiven
FunctionPrompt.taskguarantees an async function at creation, thecast(Callable[..., Awaitable[Any]], prompt.fn)cleanly satisfies Docket’s typing without changing behavior. Looks good.src/fastmcp/server/context.py (1)
391-392: Targeted type ignores are reasonable for dynamic MCP internalsThe
session._fastmcp_idassignment and passingself.request_contextinto the sampling handler are both existing behaviors; the addedtype: ignorecomments simply acknowledge the dynamic attributes/signature and keep the code type-checkable without altering runtime semantics.Also applies to: 544-544
src/fastmcp/cli/cli.py (2)
100-107: Version root-path guard is a safe improvementUsing
Path(fastmcp.__file__ or ".")avoids thePath(None)edge case when__file__is missing/falsey, without affecting typical installs where it is set. Change looks good.
822-830: Asserts inproject preparenicely document non-None assumptionsGiven earlier error handling already exits when
config_pathoroutput_dirareNone, the addedassertstatements are purely for typing clarity and won’t change behavior; they’re acceptable here.src/fastmcp/server/elicitation.py (1)
40-48: Enum inlining override with localized type ignores looks correctOverriding
generate_innerto special-case enum schemas and callingenum_schemadirectly achieves the desired “no refs for enums” behavior; the addedtype: ignoreannotations just reconcile this with Pydantic’s types and don’t affect runtime.src/fastmcp/utilities/mcp_server_config/v1/mcp_server_config.py (1)
181-196:validate_sourcetype ignore matches intended flexible Source handlingReturning
vwith# type: ignore[return-value]is consistent with the function’s docstring: it passes through existingSourceinstances while constructingFileSystemSourcefrom dicts. This keeps runtime behavior flexible while satisfying the narrowerSourceTypealias for static checking.src/fastmcp/utilities/components.py (1)
99-121: Copy/model_copy overrides correctly manage private attributesThe customized
model_copyandcopyimplementations work around Pydantic’s private-attr update limitations while keeping the public API intact:_keyand_mirroredare set explicitly on the cloned instance, and thetype: ignore[override]annotations are confined to typing concerns. This is a sensible and clear approach.Also applies to: 140-143, 176-181
src/fastmcp/utilities/json_schema_type.py (1)
251-252: Remove unused# noqa: UP007while keeping necessary type ignoresThe added
type: ignore[...]annotations are fine to satisfy stricter typing without changing behavior. However, the# noqa: UP007on thecombined = Union[tuple(item_types)]line is unused and flagged by Ruff (RUF100); you can drop just that directive:- combined = Union[tuple(item_types)] # type: ignore[arg-type] # noqa: UP007 + combined = Union[tuple(item_types)] # type: ignore[arg-type]This keeps the type suppression you need while silencing the linter.
Also applies to: 268-269, 285-285
⛔ Skipped due to learnings
Learnt from: CR Repo: jlowin/fastmcp PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-12-04T00:17:41.238Z Learning: Applies to tests/**/*.py : Use `# type: ignore[attr-defined]` in tests for MCP results instead of type assertionssrc/fastmcp/server/auth/oauth_proxy.py (2)
1009-1010: Type annotation workaround for ty upgrade.The
type: ignore[arg-type]annotation accommodates stricter type checking in ty 0.0.1-alpha.31 for theAuthorizeErrorerror parameter. The error code "invalid_client" is a valid OAuth 2.0 error code per RFC 6749 Section 5.2, and runtime behavior is unaffected.
1517-1517: Return type covariance workaround consistent with other providers.The
type: ignore[override]annotation handles ty's stricter checking on return type covariance. This method returnsAccessToken | Nonewhere the parent class expects a different signature. The same pattern appears in other auth providers (in_memory.py line 286, jwt.py line 366), confirming this is the standard workaround for the ty upgrade.src/fastmcp/client/tasks.py (1)
204-204: Type narrowing workaround for fire-and-forget async callback.The
type: ignore[arg-type]annotation addresses ty's inability to narrow the type ofresulttoAwaitable[None]after theinspect.isawaitable(result)runtime check. The fire-and-forget pattern is correct for status notification callbacks, and the runtime guard ensures type safety.src/fastmcp/client/messages.py (1)
38-49: Temporary workarounds for match statement type narrowing limitations.The
type: ignoreannotations throughout the match statement address ty's current inability to narrow union types within match cases. The TODO(ty) comments appropriately mark these as temporary until ty adds support for match statement narrowing. The logic correctly handles all message types, and runtime behavior is unaffected.src/fastmcp/server/proxy.py (3)
367-369: Override annotation for proxy-specific factory method signature.The
type: ignore[override]annotation handles the signature difference wherefrom_mcp_templateadds aclientparameter not present in the parent class. This is necessary for the proxy pattern where remote templates require a client connection.
571-572: Workaround for isinstance exclusion narrowing limitation.The
type: ignore[arg-type]annotation addresses ty's inability to narrow types after anisinstancecheck that excludes specific types (line 566 checks forResourceLink | EmbeddedResourceand raises). The runtime guard ensures type safety, and the TODO comment appropriately marks this as temporary.
588-595: Correctly handles URL-based elicitation introduced in MCP SDK update.The new logic properly distinguishes between form-based elicitation (with
requestedSchema) and URL-based elicitation (without schema). Using an empty object schema{"type": "object", "properties": {}}for URL-based elicitation aligns with the pattern insrc/fastmcp/server/context.pylines 640-641 whereresponse_type=Nonegenerates the same empty schema. This ensures the proxy correctly forwards both elicitation types to connected clients.src/fastmcp/client/elicitation.py (2)
29-30: Well-documented type update for URL elicitation support.The addition of
| Noneto the response type parameter, along with the clarifying comment, properly supports URL-based elicitation where no structured response schema is expected. This aligns with the MCP SDK's distinction between form-based and URL-based elicitation introduced in the upgrade.
47-54: Correctly implements URL elicitation support with proper schema handling.The logic properly distinguishes between:
- Form-based elicitation with schema: Derives response type from
params.requestedSchemaviajson_schema_to_type- Form-based elicitation without schema: Empty object schema
{"type": "object", "properties": {}}maps toNoneresponse type- URL-based elicitation: No schema available, uses
Noneresponse typeThis implementation aligns with the proxy handler in
src/fastmcp/server/proxy.pylines 588-595 and correctly handles the new elicitation variants introduced in the MCP SDK upgrade.
Test Failure AnalysisSummary: Two tests are crashing pytest-xdist workers on Windows Python 3.10, likely due to event loop conflicts in TestClient or RSA key generation. Root Cause: The tests are failing on Windows Python 3.10 with pytest-xdist workers. This is a known platform-specific issue related to how pytest-xdist handles asyncio event loops on Windows. The issue is NOT caused by the ty upgrade changes in this PR. Suggested Solutions:
In def test_successful_authorization_not_enhanced(self, oauth_proxy):
# ...
asyncio.run(oauth_proxy.register_client(client_info))This pattern is problematic under pytest-xdist on Windows. Consider converting to an async test or using a different approach for client registration that does not require Detailed AnalysisFailed Tests:
Error Pattern: Platform-Specific Issue:
Technical Background:
Why This PR Did Not Cause It: Recent Test History: Related Files
Recommendation: Re-run the workflow. These failures are environmental and not related to the ty upgrade changes. Note: This is not related to the ty upgrade changes - the failing tests were not modified in this PR. This is a pre-existing test flakiness issue specific to Windows Python 3.10 + pytest-xdist. Sources:
Updated with more accurate analysis after investigating the actual failing test code. |
1 participant
Upgrades ty compatibility from 0.0.1-alpha.29 to 0.0.1-alpha.31, which introduced stricter type checking.
Source code fixes:
Path(None)in CLIisinstancechecks for URL elicitation (new in MCP SDK)cast()for validated async functions in docket registrationty bug workarounds (with
TODO(ty):comments for future cleanup):messages.py)isinstanceunion narrowing (roots.py)Unpack[TypedDict]inference (transports.py)inoperator onstr | bytes(test files)Middlewaretyping (third-party stub issue)Test fixes: