fix: handle either $defs or definitions field when unmarshaling ToolArgumentsSchema #645
Conversation
…rgumentsSchema Older versions of the JSON Schema spec use the "definitions" field, while newer versions use "$defs". This can be seen in the Honeycomb MCP, which uses "definitions". This commit adds support for both fields, and prefers "$defs" over "definitions" if both are present.
WalkthroughAdds custom JSON deserialization support to ToolArgumentsSchema to handle both JSON Schema draft-07 "definitions" and 2019-09+ "$defs" fields, mapping both into the Defs field with appropriate precedence rules, supported by comprehensive unit tests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (1)
mcp/tools_test.go (1)
1553-1684: Good coverage of$defs/definitionsbehavior on ToolArgumentsSchemaThese tests nicely pin down the intended semantics:
TestToolArgumentsSchema_UnmarshalWithDefinitionsand_WithDefscover both draft‑07definitionsand 2019‑09+$defs.TestToolArgumentsSchema_UnmarshalPrefersDefsverifies the precedence rule.TestToolArgumentsSchema_MarshalRoundTripchecks that$defssurvives a marshal/unmarshal cycle.All of that aligns with the new UnmarshalJSON implementation and gives clear regression protection.
If you want extra safety later, you could add an integration‑style test that unmarshals a full
ToolJSON payload whoseinputSchemausesdefinitions/$defs, to ensure the wiring fromTool→ToolInputSchema→ToolArgumentsSchemabehaves as expected, but that’s an optional enhancement rather than a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp/tools.go(1 hunks)mcp/tools_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
mcp/tools_test.gomcp/tools.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
mcp/tools_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in mark3labs/mcp-go handles both InputSchema and RawInputSchema formats. When unmarshaling JSON, it first tries to parse into a structured ToolInputSchema format, and if that fails, it falls back to using the raw schema format, providing symmetry with the MarshalJSON method.
📚 Learning: 2025-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in mark3labs/mcp-go handles both InputSchema and RawInputSchema formats. When unmarshaling JSON, it first tries to parse into a structured ToolInputSchema format, and if that fails, it falls back to using the raw schema format, providing symmetry with the MarshalJSON method.
Applied to files:
mcp/tools_test.gomcp/tools.go
📚 Learning: 2025-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Applied to files:
mcp/tools_test.gomcp/tools.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Applied to files:
mcp/tools_test.gomcp/tools.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
mcp/tools_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
mcp/tools_test.go
🧬 Code graph analysis (1)
mcp/tools_test.go (1)
mcp/tools.go (3)
ToolArgumentsSchema(625-630)Properties(1166-1170)Required(908-912)
🔇 Additional comments (1)
mcp/tools.go (1)
656-679: Let me try the shell script again:
<function_calls>
#!/bin/bashFind type definitions for ToolInputSchema, ToolOutputSchema, and ToolArgumentsSchema
rg -n 'type ToolInputSchema|type ToolOutputSchema|type ToolArgumentsSchema' -A2
</function_calls>
2 participants
Older versions of the JSON Schema spec use the "definitions" field,
while newer versions use "$defs". This can be seen in the Honeycomb
MCP, which uses "definitions".
This commit adds support for both fields, and prefers "$defs" over
"definitions" if both are present.
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.