test: add regression tests for tool schema serialization (#671) - #739
Merged
Merged
Conversation
Add tests to verify that Tool inputSchema serialization does not double-wrap the schema inside an extra "properties" key, as reported in issue mark3labs#671. Three test functions cover: - Direct marshaling with various tool construction methods - Marshal/unmarshal round-trip consistency - ListToolsResult serialization (the actual server response path) Closes mark3labs#671
Contributor
WalkthroughAdds three test cases to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (2)
mcp/tools_test.go (2)
2338-2339: Consider using guarded type assertions for robustness.These unguarded type assertions will panic if the structure is unexpected. Using
require.Truewith theokidiom would provide clearer test failure messages.🔧 Suggested fix
- schema1 := parsed1["inputSchema"].(map[string]any) - schema2 := parsed2["inputSchema"].(map[string]any) + schema1, ok := parsed1["inputSchema"].(map[string]any) + require.True(t, ok, "parsed1 inputSchema should be a map") + schema2, ok := parsed2["inputSchema"].(map[string]any) + require.True(t, ok, "parsed2 inputSchema should be a map")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/tools_test.go` around lines 2338 - 2339, Replace the unguarded type assertions for parsed1 and parsed2 when creating schema1 and schema2 with the "ok" idiom and test assertions: check that parsed1["inputSchema"] and parsed2["inputSchema"] are maps using the two-value assertion (v, ok := parsedX["inputSchema"].(map[string]any)) and use require.True(ok, ...) (or require.IsType/require.NotNil as preferred) to produce clear test failures instead of allowing a panic; update the references to schema1 and schema2 to use the guarded variables after the assertion succeeds.
2369-2378: Multiple unguarded type assertions could panic on unexpected structures.Using the
okidiom withrequire.Truewould provide clearer failure messages if the JSON structure changes unexpectedly.🔧 Suggested fix for key assertions
- tools := parsed["tools"].([]any) + tools, ok := parsed["tools"].([]any) + require.True(t, ok, "tools should be an array") require.Len(t, tools, 1) - toolMap := tools[0].(map[string]any) - inputSchema := toolMap["inputSchema"].(map[string]any) + toolMap, ok := tools[0].(map[string]any) + require.True(t, ok, "tool should be a map") + inputSchema, ok := toolMap["inputSchema"].(map[string]any) + require.True(t, ok, "inputSchema should be a map") assert.Equal(t, "object", inputSchema["type"], "inputSchema.type should be 'object'") - props := inputSchema["properties"].(map[string]any) + props, ok := inputSchema["properties"].(map[string]any) + require.True(t, ok, "properties should be a map")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/tools_test.go` around lines 2369 - 2378, The test uses unguarded type assertions for parsed["tools"], tools[0], inputSchema and properties which can panic; replace each direct assertion with the comma-ok form and assert the ok with require.True (or require.IsType) to provide clear failures: e.g., check parsed["tools"] returns a []any ok, then require.Len on that slice; check tools[0] is a map[string]any into toolMap with ok and require.True; check toolMap["inputSchema"] is a map[string]any into inputSchema with ok and require.True; and check inputSchema["properties"] is a map[string]any into props with ok and require.True, keeping the existing assert.Equal for inputSchema["type"] after verifying the type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcp/tools_test.go`:
- Around line 2338-2339: Replace the unguarded type assertions for parsed1 and
parsed2 when creating schema1 and schema2 with the "ok" idiom and test
assertions: check that parsed1["inputSchema"] and parsed2["inputSchema"] are
maps using the two-value assertion (v, ok :=
parsedX["inputSchema"].(map[string]any)) and use require.True(ok, ...) (or
require.IsType/require.NotNil as preferred) to produce clear test failures
instead of allowing a panic; update the references to schema1 and schema2 to use
the guarded variables after the assertion succeeds.
- Around line 2369-2378: The test uses unguarded type assertions for
parsed["tools"], tools[0], inputSchema and properties which can panic; replace
each direct assertion with the comma-ok form and assert the ok with require.True
(or require.IsType) to provide clear failures: e.g., check parsed["tools"]
returns a []any ok, then require.Len on that slice; check tools[0] is a
map[string]any into toolMap with ok and require.True; check
toolMap["inputSchema"] is a map[string]any into inputSchema with ok and
require.True; and check inputSchema["properties"] is a map[string]any into props
with ok and require.True, keeping the existing assert.Equal for
inputSchema["type"] after verifying the type.
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.
Description
Add regression tests to verify that Tool
inputSchemaserialization produces correct JSON Schema output without double-wrapping the schema inside an extra"properties"key, as reported in #671.The issue described a scenario where
inputSchemawould be incorrectly serialized as:{ "properties": { "type": "object", "properties": { ... }, "required": [...] } }instead of the correct:
{ "type": "object", "properties": { ... }, "required": [...] }While the maintainer confirmed the bug could not be reproduced on the current codebase (and the root cause was likely addressed by #713 which added proper
MarshalJSON/UnmarshalJSONforToolInputSchema), there were no regression tests guarding against this specific failure mode.This PR adds three test functions:
TestToolInputSchema_NoDoubleWrapping_Issue671- Table-driven test covering 4 tool construction methods (WithString params, mixed types, no params, manually constructed schema), each verifying the serializedinputSchemahastypeat the top level andpropertiesdoes not contain schema-level keys.TestToolSchema_MarshalUnmarshal_RoundTrip_Issue671- Verifies marshal -> unmarshal -> marshal produces structurally identicalinputSchema.TestListToolsResult_Schema_Issue671- Verifies correct serialization when tools are nested insideListToolsResult(the actual server response path).Fixes #671
Type of Change
Checklist
Summary by CodeRabbit