- Notifications
You must be signed in to change notification settings - Fork897
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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
Uh oh!
There was an error while loading.Please reload this page.
145357a to4e1bd11Compare4e1bd11 tobb4de60CompareThere 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 theReadOnlyHint 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 |
a58937c intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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: