Skip to content

Conversation

stephentoub
Copy link
Contributor

Closes #697

cc: @asklar

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

We should also test that adding a static method using these overloads works.

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I looked at the test failures and it appears that this new WithTools<TToolType>(TToolType target, ...) overload is now being preferred over the WithTools(IEnumerable<McpServerTool> tools) if the compile-time type of the tools being passed is something like McpServerTool[] or List<McpServerTool> instead of exactly IEnumerable<McpServerTool>.

@stephentoub
Copy link
Contributor Author

I looked at the test failures and it appears that this new WithTools<TToolType>(TToolType target, ...) overload is now being preferred over the WithTools(IEnumerable<McpServerTool> tools) if the compile-time type of the tools being passed is something like McpServerTool[] or List<McpServerTool> instead of exactly IEnumerable<McpServerTool>.

Ouch. Not sure why I didn't see those failures when I ran tests locally, but yeah, that's an issue. We can't constraint the TToolType, because any type could legitimately have attributed methods on it. Seems like we probably just want to give it a different name.

@halter73
Copy link
Contributor

A different name for is probably the best bet to avoid the ambiguity, but I'm not sure what we'd call it. Maybe WithToolsFromInstance and WithToolsFromFactory? That would imply we aren't adding static tools though.

I wonder if we could raise an error earlier in cases like these, and throw immediately if WithTools<TToolType> on a type didn't add any tools because no methods had the appropriate attribute. This would go for both the existing version that takes just the generic type parameter and the new instance-based version.

I don't think this would make sense for when Assembly or Type objects are passed as runtime parameters, since you might be programmatically adding a bunch of stuff you're not sure will have any handlers, but it would be very strange to register a type using compile-time generic type parameters without any handlers. It's too bad we cannot make this a compile-time error, but I think a runtime error is probably better than nothing.

@halter73
Copy link
Contributor

How would it be to do an as IEnumerable<McpServerTool> cast of the TToolType in the new overload and call the old one?

@stephentoub
Copy link
Contributor Author

How would it be to do an as IEnumerable<McpServerTool> cast of the TToolType in the new overload and call the old one?

I went with this suggestion. Initially it felt wrong to me, but then I started thinking about it as a feature, where the TType gets to be a provider of tools if it chooses to implement that interface. Plus, an arbitrary type really wouldn't have any reason to implement that interface unless it wanted this behavior, I think. So, let's try it.

I didn't address any of the other argument validation questions. We can address that separately if desired.

@halter73 halter73 merged commit 2fa658e into modelcontextprotocol:main Aug 26, 2025
20 of 28 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.

IMcpServerBuilder WithTools cannot pass parameters to tool constructor

3 participants