-
Notifications
You must be signed in to change notification settings - Fork 286
mcp/tool: duplicate tools should not error #295
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
Conversation
findleyr
left a comment
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.
Thanks!
mcp/mcp_test.go
Outdated
| _, cs := basicConnection(t, func(s *Server) { | ||
| tool := &Tool{Name: "dup", InputSchema: &jsonschema.Schema{}} | ||
| s.AddTool(tool, nopHandler) | ||
| s.AddTool(tool, nopHandler) |
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.
The bug was only in the json schema resolution, right? Meaning, it would also be reprouced if we use a different Tool instance? In that case, use a different description and assert that the tool value is the last tool that was added, by checking its description.
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.
Good suggestion. Made the change and tests are passing.
mcp/tool.go
Outdated
| if orig == nil { | ||
| return nil, nil | ||
| } | ||
| b, err := json.Marshal(orig) |
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'm not sure if this is the best way to handle this, but @jba will know. Added him as a reviewer.
jba
left a comment
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.
Thank you!
mcp/tool: duplicate tools should not error
These changes touch SetSchema, since that was the best place that I could find resolve this issue.
This is my first time contributing to open source, I hope that I've followed expected formatting and if not I would happy to fix whatever you all need :)
All tests are passing and I duplicated tools in one of the examples(was hello/) and it ran without error.
Fixes #217