-
Notifications
You must be signed in to change notification settings - Fork 722
feat: provide tool annotations #308
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
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 implements tool annotations to enhance the human-readable titles and descriptions for each tool in the GitHub MCP Server. Key changes include updating the mcp-go version in third-party licenses, adding the mcp.WithToolAnnotation field in multiple tool definitions, and refining tool descriptions for clarity.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
third-party-licenses.*.md | Updated mcp-go license version to v0.21.1 |
pkg/toolsets/toolsets.go | Added explicit annotation settings for write/read tools |
pkg/github/*.go | Added mcp.WithToolAnnotation to enhance tool specifications |
pkg/github/context_tools.go | Added tool annotation for GetMe with a typo in the title |
Files not reviewed (1)
- go.mod: Language not supported
145357a
to
4e1bd11
Compare
4e1bd11
to
bb4de60
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.
Looks good to me, the only thing that jumps out to me is that the logic for whether something is read-only now lives in two places. It would be my preference to have the toolset registration use the hints here, so that the state is declarative; this will make it easier to use for docs generation.
for _, tool := range tools { | ||
if tool.Tool.Annotations.ReadOnlyHint { | ||
panic(fmt.Sprintf("tool (%s) is incorrectly annotated as read-only", tool.Tool.Name)) | ||
} | ||
} |
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 this should probably be tested, but if you're open to it I'd like to flip this and see if we can use the ReadOnlyHint
declaratively to build the toolsets?
Bypassing the "Merging is blocked" since I pushed last because the only thing I did was fix merge conflicts on the |
Closes: #306
We can provide tool annotations as part of the tool specification. This PR sets the descriptions (with ability to override translations), and the
ReadOnlyHint
.I tried to make the descriptions more human readable as intended also. These are the translation file entries: