fix: ToolInputSchema inherits MarshalJSON from ToolArgumentsSchema#696
Open
proyuen wants to merge 1 commit intomark3labs:mainfrom
Open
fix: ToolInputSchema inherits MarshalJSON from ToolArgumentsSchema#696proyuen wants to merge 1 commit intomark3labs:mainfrom
proyuen wants to merge 1 commit intomark3labs:mainfrom
Conversation
Fixes mark3labs#694 Changed ToolInputSchema and ToolOutputSchema from type definitions to type aliases so they properly inherit the MarshalJSON method from ToolArgumentsSchema. This ensures the 'properties' field is always included in JSON output, even when empty. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
WalkthroughThis PR converts Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mcp/tools.go`:
- Around line 645-646: Add GoDoc comments for the exported type aliases: prepend
a comment starting with "ToolInputSchema" above the declaration "type
ToolInputSchema = ToolArgumentsSchema" and a comment starting with
"ToolOutputSchema" above "type ToolOutputSchema = ToolArgumentsSchema" that
briefly explains they are retro-compatible aliases of ToolArgumentsSchema and
inherit its methods; follow the project's style used for other aliases (e.g.,
OAuthConfig, Token) so the comments begin with the identifier name and
succinctly describe purpose/compatibility.
🧹 Nitpick comments (1)
mcp/tools_test.go (1)
1768-1801: Consider consolidating into a table-driven test to reduce duplication.These two tests are structurally identical and can be merged into a single table-driven test for consistency and easier extension.
♻️ Proposed refactor
-// TestToolInputSchema_MarshalJSON_EmptyProperties verifies that ToolInputSchema -// inherits the MarshalJSON method from ToolArgumentsSchema, ensuring that -// the 'properties' field is included in JSON output even when empty. -// This fixes issue `#694`. -func TestToolInputSchema_MarshalJSON_EmptyProperties(t *testing.T) { - schema := ToolInputSchema{ - Type: "object", - Properties: map[string]any{}, // empty but not nil - } - - b, err := json.Marshal(schema) - assert.NoError(t, err) - - result := string(b) - // Ensure 'properties' field is present in JSON output - assert.Contains(t, result, `"properties"`, "Expected properties field in JSON output") - // Verify the full expected output - assert.Contains(t, result, `"properties":{}`, "Expected empty properties object in JSON output") -} - -// TestToolOutputSchema_MarshalJSON_EmptyProperties verifies the same for ToolOutputSchema. -func TestToolOutputSchema_MarshalJSON_EmptyProperties(t *testing.T) { - schema := ToolOutputSchema{ - Type: "object", - Properties: map[string]any{}, // empty but not nil - } - - b, err := json.Marshal(schema) - assert.NoError(t, err) - - result := string(b) - assert.Contains(t, result, `"properties":{}`, "Expected empty properties object in JSON output") -} +// TestToolSchema_MarshalJSON_EmptyProperties verifies both ToolInputSchema and +// ToolOutputSchema marshal empty properties maps as {}. +func TestToolSchema_MarshalJSON_EmptyProperties(t *testing.T) { + tests := []struct { + name string + schema any + }{ + { + name: "input_schema", + schema: ToolInputSchema{ + Type: "object", + Properties: map[string]any{}, + }, + }, + { + name: "output_schema", + schema: ToolOutputSchema{ + Type: "object", + Properties: map[string]any{}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b, err := json.Marshal(tt.schema) + assert.NoError(t, err) + result := string(b) + assert.Contains(t, result, `"properties":{}`, "Expected empty properties object in JSON output") + }) + } +}As per coding guidelines, tests should follow a table-driven structure where practical.
Comment on lines
+645
to
+646
| type ToolInputSchema = ToolArgumentsSchema // For retro-compatibility (true alias, inherits methods) | ||
| type ToolOutputSchema = ToolArgumentsSchema |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current state of mcp/tools.go around lines 645-646
sed -n '640,650p' mcp/tools.goRepository: mark3labs/mcp-go
Length of output: 583
🏁 Script executed:
# Search for context around ToolInputSchema and ToolOutputSchema
rg -B 3 -A 3 'type ToolInputSchema|type ToolOutputSchema' mcp/tools.goRepository: mark3labs/mcp-go
Length of output: 424
🏁 Script executed:
# Check if there are GoDoc comments anywhere above these lines
sed -n '630,650p' mcp/tools.goRepository: mark3labs/mcp-go
Length of output: 852
🏁 Script executed:
# Check how ToolInputSchema and ToolOutputSchema are used in the codebase
rg 'ToolInputSchema|ToolOutputSchema' mcp/tools.go -B 2 -A 2Repository: mark3labs/mcp-go
Length of output: 1108
🏁 Script executed:
# Check if there are any other type alias examples in the codebase with GoDoc comments
rg 'type \w+ = \w+' --type go -B 2 -A 0Repository: mark3labs/mcp-go
Length of output: 1111
Add GoDoc comments for the exported aliases.
ToolInputSchema and ToolOutputSchema are exported type aliases but lack GoDoc comments. Other type aliases in the codebase (e.g., OAuthConfig, Token in client/oauth.go) follow the pattern of having GoDoc comments starting with the identifier name. Align these aliases with the project's documentation standard.
✍️ Proposed fix
+// ToolInputSchema is an alias of ToolArgumentsSchema for backward compatibility.
+// It inherits ToolArgumentsSchema's JSON marshaling behavior.
type ToolInputSchema = ToolArgumentsSchema // For retro-compatibility (true alias, inherits methods)
+
+// ToolOutputSchema is an alias of ToolArgumentsSchema for backward compatibility.
+// It inherits ToolArgumentsSchema's JSON marshaling behavior.
type ToolOutputSchema = ToolArgumentsSchema📝 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.
Suggested change
| type ToolInputSchema = ToolArgumentsSchema // For retro-compatibility (true alias, inherits methods) | |
| type ToolOutputSchema = ToolArgumentsSchema | |
| // ToolInputSchema is an alias of ToolArgumentsSchema for backward compatibility. | |
| // It inherits ToolArgumentsSchema's JSON marshaling behavior. | |
| type ToolInputSchema = ToolArgumentsSchema // For retro-compatibility (true alias, inherits methods) | |
| // ToolOutputSchema is an alias of ToolArgumentsSchema for backward compatibility. | |
| // It inherits ToolArgumentsSchema's JSON marshaling behavior. | |
| type ToolOutputSchema = ToolArgumentsSchema |
🤖 Prompt for AI Agents
In `@mcp/tools.go` around lines 645 - 646, Add GoDoc comments for the exported
type aliases: prepend a comment starting with "ToolInputSchema" above the
declaration "type ToolInputSchema = ToolArgumentsSchema" and a comment starting
with "ToolOutputSchema" above "type ToolOutputSchema = ToolArgumentsSchema" that
briefly explains they are retro-compatible aliases of ToolArgumentsSchema and
inherit its methods; follow the project's style used for other aliases (e.g.,
OAuthConfig, Token) so the comments begin with the identifier name and
succinctly describe purpose/compatibility.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ToolInputSchemaandToolOutputSchemafrom type definitions to type aliasesMarshalJSONmethod fromToolArgumentsSchemapropertiesfield is now always included in JSON output, even when emptyFixes #694
Problem
ToolInputSchemawas defined as:This meant it didn't inherit
MarshalJSON, causingpropertiesto be omitted when empty.Solution
Changed to a type alias:
Test plan
TestToolInputSchema_MarshalJSON_EmptyPropertiestestTestToolOutputSchema_MarshalJSON_EmptyPropertiestestgo test ./...)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.