-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Add tool annotations to MongoDB tools for improved LLM understanding #2219
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?
feat: Add tool annotations to MongoDB tools for improved LLM understanding #2219
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @triepod-ai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates Model Context Protocol (MCP) tool annotations into the MongoDB toolset. By adding Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request consistently adds readOnlyHint and destructiveHint annotations to all 9 MongoDB tools, which is a valuable enhancement for LLM interaction. The implementation correctly adds a new Annotations field to each tool's Config struct and provides appropriate default values. The changes are well-aligned with the PR's goal. My main feedback is regarding the code duplication for setting the default annotations, which is repeated across all modified files. I've left a comment with a suggestion to refactor this into centralized helper functions to improve long-term maintainability. Overall, this is a good feature addition.
| // Add default annotations | ||
| annotations := cfg.Annotations | ||
| if annotations == nil { | ||
| readOnlyHint := true | ||
| annotations = &tools.ToolAnnotations{ | ||
| ReadOnlyHint: &readOnlyHint, | ||
| } | ||
| } |
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.
This logic for setting default annotations is duplicated across the 9 MongoDB tool files in this pull request. While the implementation is correct, and I see from the PR description that this follows an existing pattern, this repetition could pose a maintenance challenge in the future. For instance, adding a new default annotation would require changes in all 9 locations.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider centralizing this logic. A potential approach is to introduce helper functions in the tools package.
For example, you could have:
// In internal/tools/tools.go
// NewReadOnlyAnnotations creates default annotations for a read-only tool.
func NewReadOnlyAnnotations() *ToolAnnotations {
readOnly := true
return &ToolAnnotations{ReadOnlyHint: &readOnly}
}
// NewDestructiveAnnotations creates default annotations for a destructive tool.
func NewDestructiveAnnotations() *ToolAnnotations {
readOnly := false
destructive := true
return &ToolAnnotations{
ReadOnlyHint: &readOnly,
DestructiveHint: &destructive,
}
}With these helpers, this block could be simplified to:
// Add default annotations
annotations := cfg.Annotations
if annotations == nil {
annotations = tools.NewReadOnlyAnnotations()
}This would apply similarly to the destructive tools using tools.NewDestructiveAnnotations().
Since this refactoring would affect files outside this PR, it could be addressed in a follow-up change to improve the overall codebase health.
1a8fb54 to
7545295
Compare
|
Thanks for the excellent feedback! I've addressed the code duplication concern by adding centralized helper functions to
All MongoDB tools have been refactored to use these helpers, reducing the per-tool annotation logic from ~8 lines to a single line: annotations := tools.GetAnnotationsOrDefault(cfg.Annotations, tools.NewReadOnlyAnnotations)This centralizes the defaults and will make adding annotations to the remaining ~140 tools much cleaner. Net result: -39 lines of code while adding the helper infrastructure. |
901f060 to
6e7725f
Compare
duwenxin99
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.
Hi @triepod-ai, thanks for adding this feature! The implementation looks good to me. Could you include tests and documentation for this? An Mcp annotation test can be added similar to the prompt test. You can add the annotation docs here as a new section. Thanks!
|
Hi @duwenxin99, Thanks for the review feedback! I've addressed the requested changes: Tests Added:
Documentation Added:
All tests pass locally. Ready for re-review! |
…nding Add readOnlyHint and destructiveHint annotations to all MongoDB tools to help LLMs better understand tool behavior and make safer decisions. Changes: - Added helper functions to tools.go: NewReadOnlyAnnotations(), NewDestructiveAnnotations(), GetAnnotationsOrDefault() - Added readOnlyHint: true to find, findone, aggregate tools - Added destructiveHint: true to insert*, update*, delete* tools - Added Annotations field to Config structs for YAML configurability The helper functions centralize annotation defaults, making it easier to add annotations to remaining tools and maintain consistency.
- Add TestAnnotations to all 9 MongoDB tool test files - Add Tool Annotations section to docs/en/resources/tools/_index.md - Tests verify default annotations and YAML override behavior
159062f to
43560cc
Compare
|
/gcbrun |
| t.Fatal("expected annotations to be set") | ||
| } | ||
| if mcpManifest.Annotations.DestructiveHint == nil || !*mcpManifest.Annotations.DestructiveHint { | ||
| t.Error("expected destructiveHint to be true for destructive tool") |
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.
why are we using t.Error instead of t.Fatal here?
Summary
Adds MCP tool annotations (
readOnlyHint,destructiveHint) to all 9 MongoDB tools to help LLMs better understand tool behavior and make safer decisions.Changes
readOnlyHint: truereadOnlyHint: truereadOnlyHint: truedestructiveHint: truedestructiveHint: truedestructiveHint: truedestructiveHint: truedestructiveHint: truedestructiveHint: trueImplementation
Each tool now:
Annotationsfield in its Config struct for YAML configurabilityGetMcpManifest()instead ofnilThis follows the exact pattern established by the Looker tools (e.g.,
lookergetconnectionschemas,lookerupdateprojectfile).Why This Matters
readOnlyHinttells LLMs a tool is safe to call without side effectsdestructiveHintsignals LLMs should be more careful before executingTesting
tools/listreturns annotations in MCP responseFiles Changed
internal/tools/mongodb/mongodbfind/mongodbfind.gointernal/tools/mongodb/mongodbfindone/mongodbfindone.gointernal/tools/mongodb/mongodbaggregate/mongodbaggregate.gointernal/tools/mongodb/mongodbinsertone/mongodbinsertone.gointernal/tools/mongodb/mongodbinsertmany/mongodbinsertmany.gointernal/tools/mongodb/mongodbupdateone/mongodbupdateone.gointernal/tools/mongodb/mongodbupdatemany/mongodbupdatemany.gointernal/tools/mongodb/mongodbdeleteone/mongodbdeleteone.gointernal/tools/mongodb/mongodbdeletemany/mongodbdeletemany.go🤖 Generated with Claude Code