-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix task execution for tools with custom names #2645
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -112,23 +112,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger = get_logger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _create_named_fn_wrapper(fn: Callable[..., Any], name: str) -> Callable[..., Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Create a wrapper function with a custom __name__ for Docket registration. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Docket uses fn.__name__ as the key for function registration and lookup. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| When mounting servers, we need unique names to avoid collisions between | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mounted servers that have identically-named functions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import functools | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @functools.wraps(fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def wrapper(*args: Any, **kwargs: Any) -> Any: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return await fn(*args, **kwargs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wrapper.__name__ = name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return wrapper | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DuplicateBehavior = Literal["warn", "error", "replace", "ignore"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Transport = Literal["stdio", "http", "sse", "streamable-http"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -441,50 +424,48 @@ async def _docket_lifespan(self) -> AsyncIterator[None]: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(tool, FunctionTool) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and tool.task_config.mode != "forbidden" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(tool.fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(tool.fn, names=[tool.key]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for prompt in self._prompt_manager._prompts.values(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(prompt, FunctionPrompt) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and prompt.task_config.mode != "forbidden" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # task execution requires async fn (validated at creation time) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(cast(Callable[..., Awaitable[Any]], prompt.fn)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cast(Callable[..., Awaitable[Any]], prompt.fn), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| names=[prompt.key], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for resource in self._resource_manager._resources.values(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(resource, FunctionResource) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and resource.task_config.mode != "forbidden" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(resource.fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(resource.fn, names=[resource.name]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for template in self._resource_manager._templates.values(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isinstance(template, FunctionResourceTemplate) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| and template.task_config.mode != "forbidden" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(template.fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(template.fn, names=[template.name]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
447
to
+452
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Template registration uses wrong lookup key. Templates are registered with Templates should be registered using 🔎 Apply this diff to fix the registration key:- docket.register(template.fn, names=[template.name])
+ docket.register(template.fn, names=[template.key])🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have tests (including some new ones) that validate that this is all good, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 305 🏁 Script executed: Length of output: 962 🏁 Script executed: Length of output: 5027 🏁 Script executed: Length of output: 13588 🏁 Script executed: Length of output: 6196 🏁 Script executed: Length of output: 1159 🏁 Script executed: Length of output: 18199 @chrisguidry Yes, you're absolutely right! The new test file The test:
The code is correct as-is. Templates are registered and looked up using
The 🧬 Code Graph Analysis Results[possibly_relevant_code_snippets] src/fastmcp/server/server.py
examples/testing_demo/tests/test_server.py
Snippet (complete): async def client(): [end of snippet] 🧠 Learnings used |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Register provider components | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for provider in self._providers: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tasks = await provider.get_tasks() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for tool in tasks.tools: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| named_fn = _create_named_fn_wrapper(tool.fn, tool.key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(named_fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(tool.fn, names=[tool.key]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for resource in tasks.resources: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| named_fn = _create_named_fn_wrapper( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resource.fn, resource.name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(named_fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(resource.fn, names=[resource.name]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for template in tasks.templates: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| named_fn = _create_named_fn_wrapper( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template.fn, template.name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(named_fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(template.fn, names=[template.name]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for prompt in tasks.prompts: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| named_fn = _create_named_fn_wrapper(prompt.fn, prompt.key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register(named_fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| docket.register( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cast(Callable[..., Awaitable[Any]], prompt.fn), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| names=[prompt.key], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
458
to
+468
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Provider resource and template registrations use wrong lookup keys. While tools and prompts correctly use 🔎 Apply this diff to fix both registration keys: for tool in tasks.tools:
docket.register(tool.fn, names=[tool.key])
for resource in tasks.resources:
- docket.register(resource.fn, names=[resource.name])
+ docket.register(resource.fn, names=[resource.key])
for template in tasks.templates:
- docket.register(template.fn, names=[template.name])
+ docket.register(template.fn, names=[template.key])
for prompt in tasks.prompts:
docket.register(
cast(Callable[..., Awaitable[Any]], prompt.fn),
names=[prompt.key],
)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we have tests (including some new ones) that validate that this is all good, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 476 🏁 Script executed: Length of output: 11266 🏁 Script executed: Length of output: 6660 🏁 Script executed: Length of output: 769 🏁 Script executed: Length of output: 5082 🏁 Script executed: Length of output: 5941 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 19501 🏁 Script executed: Length of output: 768 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 79 🏁 Script executed: Length of output: 46 🏁 Script executed: Length of output: 46 🏁 Script executed: Length of output: 81 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 229 🏁 Script executed: Length of output: 131 @chrisguidry Looking at the code, I think the current implementation is actually correct and intentional. Here's what I found: The pattern is consistent across the codebase:
There's even an explicit comment in # Use resource.name which matches what was registered - prefixed for mounted resourcesThe distinction is:
This appears to be an intentional design choice, not a bug. If the review comment were correct, then both local and provider registrations would be wrong, not just provider. However, I couldn't find specific integration tests that verify provider resource/template task execution works correctly. While the pattern looks intentional and consistent, having tests that cover:
would provide additional confidence. Did you add such tests in the new 🧬 Code Graph Analysis ResultsFile: examples/testing_demo/tests/test_server.py async def client():
"""
Client fixture for testing.
Uses async context manager and yields client synchronously.
No @pytest.mark.asyncio needed - asyncio_mode = "auto" handles it.
"""
# Import here to avoid import-time side effects
from server import mcp
async with Client(mcp) as client:
yield clientFile: src/fastmcp/server/tasks/capabilities.py """ 🧠 Learnings used |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| provider_name = getattr( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| provider, "server", provider | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Resource registration uses wrong lookup key.
Resources are registered with
resource.namebut looked up by URI during task execution (see line 659-661:handle_resource_as_task(self, str(uri), ...)). This recreates the original bug from #2642 where the registration key doesn't match the lookup key, causing task execution to fail.Resources should be registered using
resource.key, which returns the URI used for lookups.🔎 Apply this diff to fix the registration key:
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have tests (including some new ones) that validate that this is all good, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 7191
🏁 Script executed:
Length of output: 92
🏁 Script executed:
Length of output: 2987
🏁 Script executed:
Length of output: 112
🏁 Script executed:
Length of output: 207
🏁 Script executed:
Length of output: 1890
🏁 Script executed:
Length of output: 138
🏁 Script executed:
Length of output: 12076
🏁 Script executed:
Length of output: 3731
🏁 Script executed:
Length of output: 1563
🏁 Script executed:
Length of output: 130
🏁 Script executed:
Length of output: 610
🏁 Script executed:
Length of output: 663
🏁 Script executed:
Length of output: 4017
🏁 Script executed:
Length of output: 562
🏁 Script executed:
Length of output: 297
@chrisguidry You're correct! Looking at the actual implementation, both the registration and lookup use
resource.name, so they match:Registration (line 445):
Lookup in handler (line 318 in
handlers.py):CodeRabbit's concern appears to be based on seeing that
handle_resource_as_task()receivesurias a parameter, but the handler actually retrieves the resource object and then usesresource.name(not the URI) for the docket lookup. The comment in the handler even explicitly states this: "Use resource.name which matches what was registered".This is consistent with how tools work - they use
tool.keyfor both registration and lookup. Resources useresource.namefor both operations.However, I couldn't find specific tests that exercise task execution with resources (or resources with custom names different from their URI). If you've added tests in this PR that validate the registration/lookup flow works correctly with custom names, that would be great confirmation! Could you point me to those tests?
🧬 Code Graph Analysis Results
File: src/fastmcp/server/tasks/capabilities.py (lines 6-22)
Summary:
Included content:
def get_task_capabilities() -> dict[str, Any]:
"""Return the SEP-1686 task capabilities structure.
File: src/fastmcp/server/middleware/middleware.py (lines 79-205)
Description: Core middleware base class and dispatch logic used by the server to process MCP requests. This defines how different MCP protocol actions (initialize, tools/call, resources/read, prompts/get, etc.) are routed through the middleware chain and into concrete handlers.
Included content:
class Middleware:
"""Base class for FastMCP middleware with dispatching hooks."""
File: src/fastmcp/server/providers/base.py (lines 72-316)
Summary:
Reason this is relevant:
Note: This is a long snippet; summarized here. If you want, I can include a condensed excerpt focusing on key methods (list_tools, get_tool, list_resources, read_resource, render_prompt, and lifespan) upon request.
🧠 Learnings used