fix(tools): omit tool annotations from JSON when unset - #911
Conversation
Add ToolAnnotation.HasAny() and only include the annotations field in Tool.MarshalJSON when at least one annotation value is present. Introduce WithoutDefaultAnnotations() so callers can opt out of the default hint pointers initialized by NewTool and omit annotations from tools/list output when they have no annotation metadata. Fixes mark3labs#710
|
Connected to Huly®: MCP_G-475 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Tool Annotations Marshaling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcp/tools_additional_test.go (1)
297-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert this new test matrix to a table-driven test.
These cases are already exercising a small annotation-state matrix, so a
tests := []struct{ ... }table will be easier to extend and keeps the file aligned with the repo’s Go test convention. As per coding guidelines, implement table-driven tests withtests := []struct{ name, ... }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools_additional_test.go` around lines 297 - 362, Refactor TestToolAnnotationsMarshalJSON into a table-driven test using a tests := []struct{ name, ... } matrix so each case is easier to extend and follows the repo’s Go test convention. Keep the existing coverage for NewTool, WithoutDefaultAnnotations, NewToolWithRawSchema, and ToolAnnotation.HasAny by moving the current subtest logic into table entries and iterating over them with t.Run. Preserve the same assertions and JSON checks while centralizing the shared marshal/unmarshal setup.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/tools.go`:
- Around line 961-964: WithoutDefaultAnnotations currently resets the entire
ToolAnnotation on Tool, which clears explicitly set metadata like the title and
can make MarshalJSON omit annotations entirely. Update
WithoutDefaultAnnotations() so it only removes the default hint pointer fields
while preserving any user-supplied annotations such as the title set by
WithTitleAnnotation, and keep the behavior localized to the ToolAnnotation
handling in that option.
---
Nitpick comments:
In `@mcp/tools_additional_test.go`:
- Around line 297-362: Refactor TestToolAnnotationsMarshalJSON into a
table-driven test using a tests := []struct{ name, ... } matrix so each case is
easier to extend and follows the repo’s Go test convention. Keep the existing
coverage for NewTool, WithoutDefaultAnnotations, NewToolWithRawSchema, and
ToolAnnotation.HasAny by moving the current subtest logic into table entries and
iterating over them with t.Run. Preserve the same assertions and JSON checks
while centralizing the shared marshal/unmarshal setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 981223fb-7d0e-4fe6-a329-2e8dba79bf97
📒 Files selected for processing (2)
mcp/tools.gomcp/tools_additional_test.go
| func WithoutDefaultAnnotations() ToolOption { | ||
| return func(t *Tool) { | ||
| t.Annotations = ToolAnnotation{} | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't clear an explicitly set annotation title here.
This resets the whole ToolAnnotation, so NewTool("test", WithTitleAnnotation("My Tool"), WithoutDefaultAnnotations()) drops the user-supplied title and MarshalJSON omits annotations entirely. WithoutDefaultAnnotations() should only strip the default hint pointers, not unrelated explicit metadata.
Suggested fix
func WithoutDefaultAnnotations() ToolOption {
return func(t *Tool) {
- t.Annotations = ToolAnnotation{}
+ title := t.Annotations.Title
+ t.Annotations = ToolAnnotation{
+ Title: title,
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithoutDefaultAnnotations() ToolOption { | |
| return func(t *Tool) { | |
| t.Annotations = ToolAnnotation{} | |
| } | |
| func WithoutDefaultAnnotations() ToolOption { | |
| return func(t *Tool) { | |
| title := t.Annotations.Title | |
| t.Annotations = ToolAnnotation{ | |
| Title: title, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/tools.go` around lines 961 - 964, WithoutDefaultAnnotations currently
resets the entire ToolAnnotation on Tool, which clears explicitly set metadata
like the title and can make MarshalJSON omit annotations entirely. Update
WithoutDefaultAnnotations() so it only removes the default hint pointer fields
while preserving any user-supplied annotations such as the title set by
WithTitleAnnotation, and keep the behavior localized to the ToolAnnotation
handling in that option.
There was a problem hiding this comment.
@ezynda3 No—the current implementation still resets the entire ToolAnnotation, so it clears a title supplied by an earlier option. For example:
NewTool("test",
WithTitleAnnotation("My Tool"),
WithoutDefaultAnnotations(),
)will omit annotations rather than retain {"title":"My Tool"}. WithoutDefaultAnnotations() should preserve t.Annotations.Title while clearing only the default hint pointers.
Address review feedback on PR mark3labs#911 by using a shared test matrix for annotation JSON marshaling cases.
ezynda3
added
the
status: needs submitter response
2 participants
Summary
Omit the
annotationsfield from tool JSON when no annotation values are set, and addWithoutDefaultAnnotations()so callers can opt out ofNewTool's default hint pointers.Motivation
Tool.MarshalJSONalways includedannotations, even for tools with no annotation metadata. Clients could not distinguish "explicitly annotated" from "no annotation data available" (see #710).Changes
ToolAnnotation.HasAny()to detect whether any annotation field is setannotationsinTool.MarshalJSONonHasAny()WithoutDefaultAnnotations()ToolOptionto clearNewTooldefault hintsTests
go test ./mcp/ ./server/ -count=1All tests pass.
Notes
NewTool()behavior is unchanged: default hint pointers are still serialized for backward compatibilityNewTool(name, WithoutDefaultAnnotations(), ...)to omit annotations fromtools/listoutput when you have no annotation metadataSummary by CodeRabbit
annotationsfield in JSON unless annotation hint values are explicitly set, resulting in cleaner output.annotationsJSON inclusion/omission behavior and helper detection of set annotation values.