-
Notifications
You must be signed in to change notification settings - Fork 101
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
Overhaul tool handling #89
Overhaul tool handling #89
Conversation
ea82109
to
ccc8b63
Compare
@colombod, please take a look. I'm not sure if this will fully meet your needs, but it's in the right direction, so my request would be after this merges, please experiment, and come back with a draft PR to address any gaps. |
@aaronpowell, please take a look, based on your feedback about supporting both a tool collection and tool handlers as fallbacks. |
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.
Pull Request Overview
This PR overhauls the tool handling functionality by renaming tool-related types and improving dependency injection support. Key changes include renaming McpTool types to McpServerTool types, updating DI registrations with WithToolsFromAssembly, and revising client APIs to return IList rather than IAsyncEnumerable.
Reviewed Changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/ModelContextProtocol/AIContentExtensions.cs | Adds AI content conversion helpers with new collection initialization syntax changes |
src/ModelContextProtocol/Client/McpClientTool.cs | Introduces a new client-side tool function wrapper |
samples/AspNetCoreSseServer/Tools/SampleLlmTool.cs | Updates tool attributes and method signatures to use server-oriented DI injections |
src/ModelContextProtocol/Configuration/McpServerOptionsSetup.cs | Adjusts options setup to integrate the new tool collection from DI |
src/ModelContextProtocol/Client/McpClientExtensions.cs | Revises listing and invoking of tools/prompts/resources with updated return types |
src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.Tools.cs | Refactors tool extension methods to use the new attribute and registration approach |
README.MD | Updates documentation snippets to reflect the renaming and new API usage |
Other sample files | Updates tool attribute names and DI registration calls |
Files not reviewed (2)
- Directory.Packages.props: Language not supported
- samples/ChatWithTools/ChatWithTools.csproj: Language not supported
Comments suppressed due to low confidence (2)
src/ModelContextProtocol/AIContentExtensions.cs:22
- The array initializer using square brackets is not standard C# syntax. Consider using a collection initializer like 'new List { ToAIContent(promptMessage.Content) }'.
Contents = [ToAIContent(promptMessage.Content)]
src/ModelContextProtocol/Client/McpClientExtensions.cs:90
- Initializing the collection 'prompts' as null can lead to null returns; consider initializing it with an empty list (e.g. 'new List()') to simplify accumulation and avoid potential null issues.
List<Prompt>? prompts = null;
Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more
@halter73, please take a look, in particular at the DI-related stuff, plus all of the lovely code required to deal with records in the options types, per our conversation earlier today. |
Overall it looks like it should tackle the issue. I'll combine it with my branch for the Everything Server and have a test. |
src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.Tools.cs
Show resolved
Hide resolved
ccc8b63
to
af7ffae
Compare
- Renames [McpTool{Type}] to [McpServerTool{Type}], in order to distinguish representations of tools on the server from tools on the client. - Enables [McpServerTool] methods to have arguments injected from DI, as well as a specific set of types from the implementation directly, like IMcpServer. - Renames WithTools to WithToolsFromAssembly. - All of the WithToolsXx methods now publish each individual McpServerTool into DI; other code can do so as well. The options setup code gathers all of the tools from DI and combines them into a collection which is then stored in McpServerOptions and used to construct the server. - The server tools specified via DI as well as manually-provided handlers, using CallToolHandler as a fallback in case the requested tool doesn't exist in the tools collection, and ListToolsHandler augments the information for whatever tools exist. - The tools are stored in McpServerOptions in a new McpServerToolCollection type, which is a thread-safe container that exposes add/removal notification. Adding/removing tools triggers a change notification that in turn sends a notification to the client about a tools update. - The ServerOptions are exposed from the server instance. - Removed cursor-based APIs from McpClientExtensions. - Changed McpClientExtensions APIs to return `Task<IList<...>` rather than `IAsyncEnumerable<...>`. I'm sure this will need to evolve further before we're "done", but it's a significant improvement from where we are now. One area I'm not super happy with is the notifying collection; that might work ok for a stdio server, where you can grab the created collection from the server's options and mutate it, but I don't know how well that's going to work for sse servers.
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 like the renames to WithToolsFromAssembly and McpServerToolType.
src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.Tools.cs
Show resolved
Hide resolved
af7ffae
to
b5fbb91
Compare
Task<IList<...>
rather thanIAsyncEnumerable<...>
.I'm sure this will need to evolve further before we're "done", but it's a significant improvement from where we are now. One area I'm not super happy with is the notifying collection; that might work ok for a stdio server, where you can grab the created collection from the server's options and mutate it, but I don't know how well that's going to work for sse servers.
Closes #26
Closes #27
Closes to #62
Contributes to #67
Closes #68
Contributes to #69
Closes #79
Contributes to #41