fix: properly marshal ToolAnnotations with false values
#260
Conversation
|
""" WalkthroughThe changes convert the boolean fields in the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
mcp/tools.go (3)
169-175: Successfully updated initialization to use pointer valuesThe initialization in
NewToolnow correctly uses theBoolPtrhelper function to create pointer values for the boolean fields. This ensures consistent behavior with the updated struct definition.However, consider adding a comment explaining why pointers are used for boolean fields to help future maintainers understand this design choice.
Annotations: ToolAnnotation{ Title: "", + // Using pointer values to ensure false values are properly marshaled ReadOnlyHint: BoolPtr(false), DestructiveHint: BoolPtr(true), IdempotentHint: BoolPtr(false), OpenWorldHint: BoolPtr(true), },
133-144: Consider adding comment explaining the reason for using pointers for boolean fieldsWhile the implementation is correct, adding documentation about why pointer types are used would make the code more maintainable. This helps other developers understand the reasoning behind this design choice.
type ToolAnnotation struct { // Human-readable title for the tool Title string `json:"title,omitempty"` + // Using *bool instead of bool ensures that explicit false values + // are preserved during JSON marshaling with the omitempty tag // If true, the tool does not modify its environment ReadOnlyHint *bool `json:"readOnlyHint,omitempty"` // If true, the tool may perform destructive updates DestructiveHint *bool `json:"destructiveHint,omitempty"` // If true, repeated calls with same args have no additional effect IdempotentHint *bool `json:"idempotentHint,omitempty"` // If true, tool interacts with external entities OpenWorldHint *bool `json:"openWorldHint,omitempty"` }
133-144: Consider implementing setter methods for a better developer experienceAs mentioned in the PR summary, implementing setter methods (e.g.,
withReadOnlyHint()) would provide a more elegant interface for SDK users. This would abstract away the pointer implementation details and improve the developer experience.Consider adding the following methods to improve the API:
// WithReadOnlyHint sets whether the tool does not modify its environment func WithReadOnlyHint(hint bool) ToolOption { return func(t *Tool) { t.Annotations.ReadOnlyHint = BoolPtr(hint) } } // WithDestructiveHint sets whether the tool may perform destructive updates func WithDestructiveHint(hint bool) ToolOption { return func(t *Tool) { t.Annotations.DestructiveHint = BoolPtr(hint) } } // WithIdempotentHint sets whether repeated calls with the same args have no additional effect func WithIdempotentHint(hint bool) ToolOption { return func(t *Tool) { t.Annotations.IdempotentHint = BoolPtr(hint) } } // WithOpenWorldHint sets whether the tool interacts with external entities func WithOpenWorldHint(hint bool) ToolOption { return func(t *Tool) { t.Annotations.OpenWorldHint = BoolPtr(hint) } }This would allow users to configure tools with a cleaner API:
tool := NewTool("my-tool", WithDescription("A tool that does something"), WithReadOnlyHint(true), WithIdempotentHint(true), )Also applies to: 169-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/inprocess_test.go(2 hunks)client/sse_test.go(3 hunks)mcp/tools.go(2 hunks)mcp/utils.go(1 hunks)server/server_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
client/sse_test.go (1)
mcp/utils.go (1)
BoolPtr(780-782)
client/inprocess_test.go (1)
mcp/utils.go (1)
BoolPtr(780-782)
server/server_test.go (1)
mcp/utils.go (1)
BoolPtr(780-782)
mcp/tools.go (1)
mcp/utils.go (1)
BoolPtr(780-782)
🔇 Additional comments (8)
mcp/utils.go (1)
778-782: Good addition of a utility function for boolean pointers.This is a clean implementation of a helper function that will ease the conversion from boolean values to boolean pointers, which is needed to properly marshal boolean fields with
falsevalues when using theomitemptyJSON tag.server/server_test.go (1)
873-877: Clean conversion to boolean pointers.The test code has been properly updated to use
mcp.BoolPtr()for all boolean hint fields in theToolAnnotationstruct. This change aligns with the fix for ensuringfalsevalues are correctly preserved during JSON marshaling.client/sse_test.go (3)
8-9: Import reordering looks good.The import statement for the
transportpackage has been appropriately reordered to follow standard library imports.
31-35: Properly updated boolean fields to use pointers.The test code has been correctly modified to use
mcp.BoolPtr()for all boolean hint fields in theToolAnnotationstruct, ensuringfalsevalues are preserved during JSON marshaling.
115-119: Assertion code correctly dereferencing boolean pointers.The test assertions have been properly updated to dereference the pointer values before comparison, allowing the tests to validate the boolean values correctly.
client/inprocess_test.go (2)
27-31: Properly updated boolean fields to use pointers.The test code has been correctly modified to use
mcp.BoolPtr()for all boolean hint fields in theToolAnnotationstruct, ensuring proper JSON marshaling behavior.
146-150: Assertion code correctly dereferencing boolean pointers.The test assertions have been properly updated to dereference the pointer values before comparison, allowing the tests to validate the boolean values correctly.
mcp/tools.go (1)
133-144: Good solution for preserving explicitfalsevalues during JSON marshalingThe change from
boolto*boolis an effective solution to ensure thatfalsevalues are properly marshaled to JSON rather than being omitted. In Go, theomitemptytag causes fields with zero values (includingfalsefor booleans) to be omitted during JSON marshaling. By using pointers, explicitfalsevalues will be included in the JSON output.
rwjblue-glean
left a comment
rwjblue-glean
left a comment
There was a problem hiding this comment.
Hmm. Could we remove omitempty instead? Wouldn't that have the result of making sure the fields are emitted even if the values are false? If so, that seems like better ergonomics to the end user of the sdk (dealing with the boolean pointers is a tad annoying for consumers IMO).
|
@robert-jackson-glean The problem with removing the From my experience in using this SDK, I think that the SDK provides all the tools but it is up to the implementations to use the SDK as they want to adhere to the specification as they like. The SDK supports annotations, the MCP spec recommends annotations but it is up to the user to decide if they want to support every annotation. In that sense, we should always allow users to not specify any annotation and not automatically set a BUT, I agree this is bad UX. I had suggested abstracting the pointer logic from the SDK users in the PR description:
I was waiting for some feedback and now I know for sure it is bad UX, I will abstract it out. I will add a new commit with this change and you can check if the UX is good. |
client/inprocess_test.go
Outdated
| mcp.WithToolAnnotation( | ||
| "Test Tool Annotation Title", | ||
| true, | ||
| false, | ||
| true, | ||
| false, | ||
| ), |
There was a problem hiding this comment.
This will be the UX for SDK users. It hides the pointer implementation from the users and it is more or less the same UX as we have now.
This follows similar WithX functions like:
WithResourceCapabilities(true, true),
WithPromptCapabilities(true),
WithToolCapabilities(true),
| ReadOnlyHint: ToBoolPtr(false), | ||
| DestructiveHint: ToBoolPtr(true), | ||
| IdempotentHint: ToBoolPtr(false), | ||
| OpenWorldHint: ToBoolPtr(true), |
There was a problem hiding this comment.
The ToBoolPtr helper function can be used for internal tests.
| // ToBoolPtr returns a pointer to the given boolean value | ||
| func ToBoolPtr(b bool) *bool { | ||
| return &b | ||
| } |
There was a problem hiding this comment.
ToBoolPtr is a more semantic name. This is fine IMO and is followed in other Go projects for the same reason, to distinguish between not specified and zero values.
|
Thanks for continuing to iterate on this with me! I kinda prefer the more explicit creation of the struct fields (that we have today on master) over the current rendition of this PR (e.g. many positional params). I understand what you were going with in the current rendition in that it follows the same sort of pattern as some of our existing In that light, let's go back to your first iteration. Though I'm not sure we actually need the // WithReadOnlyHintAnnotation sets the ReadOnlyHint field of the Tool's Annotations.
// If true, it indicates the tool does not modify its environment.
func WithReadOnlyHintAnnotation(value bool) ToolOption {
return func(t *Tool) {
t.Annotations.ReadOnlyHint = &value
}
}
// WithDestructiveHintAnnotation sets the DestructiveHint field of the Tool's Annotations.
// If true, it indicates the tool may perform destructive updates.
func WithDestructiveHintAnnotation(value bool) ToolOption {
return func(t *Tool) {
t.Annotations.DestructiveHint = &value
}
}
// WithIdempotentHintAnnotation sets the IdempotentHint field of the Tool's Annotations.
// If true, it indicates repeated calls with the same arguments have no additional effect.
func WithIdempotentHintAnnotation(value bool) ToolOption {
return func(t *Tool) {
t.Annotations.IdempotentHint = &value
}
}
// WithOpenWorldHintAnnotation sets the OpenWorldHint field of the Tool's Annotations.
// If true, it indicates the tool interacts with external entities.
func WithOpenWorldHintAnnotation(value bool) ToolOption {
return func(t *Tool) {
t.Annotations.OpenWorldHint = &value
}
}
// WithTitleAnnotation sets the Title field of the Tool's Annotations.
// It provides a human-readable title for the tool.
func WithTitleAnnotation(title string) ToolOption {
return func(t *Tool) {
t.Annotations.Title = title
}
}(then we just need ot make sure to tweak What do you think? Sorry for the back and forth on approach here |
Yes, I agree. I originally planned to use the helper functions, but this would mean we have two different ways to set the annotations, i.e., directly through the struct and through the helper functions. That's why I thought about let's just pass these as arguments. But yeah, too many params. Maybe I'm overthinking it and we should just add the helper functions. That's the only reasonable alternative I found. Let me update the changes. No problems with the back and forth at all. I needed help to think this through. And honestly, reviews have been really quick 🚀 |
We still need to create something like Also the helper function will only be used internally and by someone not wanting to use the helper functions right? Do you have any alternate ideas? |
I was just thinking that it's just as easy for folks to type |
Yes I understand but you can't directly get the address of a literal right? You still need a variable? |
Ha! Not sure, I just assumed you could (without a variable). |
2 participants
Fixes #259
To prevent omitting boolean values when they are
false, we can use a boolean pointer instead.Ref: https://stackoverflow.com/questions/37756236/json-golang-boolean-omitempty
This PR fixes the issue but maybe it can be made more elegant with proper setter functions (like
withReadOnlyHint()which could take in aboolinstead of a*boolto abstract the implementation from the SDK users) I will let others comment on how this should be done or whether it should be done at all.But the PR in its current form, fixes the issue.
See #260 (comment) and my review below on why these changes were made.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor