-
Notifications
You must be signed in to change notification settings - Fork 326
limit tool name length #69
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
base: main
Are you sure you want to change the base?
limit tool name length #69
Conversation
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.
First of all - thank you so much for contributing!
I had a few comments on the code itself, but also wanted to remark on the implementation in general.
I think hard-coding skipping on tools is not a good practice - even if some clients don't accept long tool names, it's not something defined by the spec so we shouldn't enforce it for all MCPs.
It will be better to implement by configuring the max length as an optional argument when generating the MCP based on the FastAPI app, roughly like this:
mcp = FastApiMCP(app, max_tool_name_length=60)
(with max_tool_name_length
default to None
).
Would be happy to see these changes implemented and then approve!
Regarding your note - it will be best to open an Issue (assigned to yourself) and then implement in a separate PR :)
Thanks again for your contribution!
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
I think syncing |
5c80302
to
2494832
Compare
@shahar4499 - Thanks, I rebased my branch with the "upstream" repo but couldn't get the I then tried to push a "dummy commit" in order to trigger it, but again, no such luck. Is there anything I'm missing? Thanks. |
8571f99
to
4388fc6
Compare
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.
Nice!
Can you address the small notes I commented?
Other than that, looks fine by me
e02d7c2
to
f4fabd6
Compare
f4fabd6
to
985a9df
Compare
Hey @shira-ayal, just pinging to see if there are any more expected changes. |
Hi @DougieHauser ! |
Hey @shahar4499, just a quick ping. Thanks. |
Describe your changes
Resolves #64
Attempts to filter tools whose operation_id + server name are too long.
Note: The codebase seems to have logic overlap between the functions
server.py/_filter_tools
andconvert.py/def convert_openapi_to_mcp_tools
- I didn't want to rock the boat too much on my first attempted contribution, but I would be happy to try my luck at refactoring (perhaps in a separate ticket).Issue ticket number and link (if applicable)
#64
Screenshots of the feature / bugfix
Checklist before requesting a review