Skip to content

Fix @mcp.tool() Failing to Detect Context Argument When Type Variables Are Used in Context #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 10, 2025

Conversation

jkawamoto
Copy link
Contributor

Motivation and Context

When defining tools to safely access the app context (lifespan context) using Context, it is necessary to include type variables.

However, the current implementation of @mcp.tool() fails to detect the context argument when type variables are specified (as detailed in #357).

This PR addresses and resolves this issue.

How Has This Been Tested?

The following commands were executed locally to verify the fix:

uv sync --frozen --all-extras --dev
uv run --frozen ruff check .
uv run --frozen pytest

Breaking Changes

No.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this solves the issue. Add a test, and you'll see.

@jkawamoto
Copy link
Contributor Author

Actually, it passes the current tests and also works on my local server.

The cause of #357 is that it only checks if param.annotation is Context.
When type variables are specialized, the resulting class is a subclass of the generic class.
Also, a class is a subclass of itself.
So, we just need to use issubclass(param.annotation, Context) instead of param.annotation is Context.

This part is for handling cases where param.annotation is typing.Literal or similar, btw:

if get_origin(param.annotation) is not None:
    continue

@jkawamoto jkawamoto requested a review from Kludex March 24, 2025 09:37
@Kludex
Copy link
Member

Kludex commented Mar 24, 2025

I'm asking to add a test where we use the generic params for Context. The context needs to be passed to the function.

This should work for:

context: Context[SessionParam]  # missing lifespan, since it has a default
context: Context[SessionParam, LifespanParam]  # both
context: Context[SessionParam, LifespanParam] | None = None  # union
context: Annotated[Context[Session, LifespanParam], "metadata"]  # Not necessary for now

@jkawamoto
Copy link
Contributor Author

I've added a test for the second case.

Regarding the first case:

Context[SessionParam]

This is not only useless but also invalid. It raises the following exception:

Too few parameters for <class 'mcp.server.fastmcp.server.Context'>; actual 1, expected 2

About the third case: I don't believe this situation should occur. If a tool requests a context, it should always be provided.

Previously, the code only checked for exact matches with `Context`. This update ensures that subclasses of `Context` are also correctly identified, improving flexibility and reliability.
@Kludex Kludex enabled auto-merge (squash) April 10, 2025 06:35
@dsp-ant dsp-ant self-requested a review April 10, 2025 09:36
@Kludex Kludex merged commit da54ea0 into modelcontextprotocol:main Apr 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants