fix: keep empty properties in tool's InputSchema #596
Conversation
OpenAI will report error if InputSchema with object type has no properties.
openai.BadRequestError: Error code: 400 - {'error': {'message': "Invalid schema for function 'getCurrentProfile': In context=(), object schema missing properties.", 'type': 'invalid_request_error', 'param': 'tools[0].function.parameters', 'code': 'invalid_function_parameters'}, 'user': '{"appkey": "egai-prd-strategyoffice-020029831-workflow-1746045106141", "session_id": "b95e59c0-a570-4f82-9905-05eff7b35eba-1758281506244515510", "user": "", "prompt_truncate": "yes"}'}
WalkthroughToolInputSchema and ToolOutputSchema in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/tools.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.go
🧠 Learnings (7)
📓 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.
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.
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
📚 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.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.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 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.go
📚 Learning: 2025-06-20T20:39:51.870Z
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
Applied to files:
mcp/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 **/*.go : For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Applied to files:
mcp/tools.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
mcp/tools.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: coverage
- GitHub Check: test
mcp/tools.go
Outdated
| Defs map[string]any `json:"$defs,omitempty"` | ||
| Type string `json:"type"` | ||
| Properties map[string]any `json:"properties,omitempty"` | ||
| Properties map[string]any `json:"properties"` |
There was a problem hiding this comment.
Struct tag change is ineffective; fix the custom MarshalJSON instead.
Removing omitempty from the struct tag won't resolve the OpenAI validation error because the custom MarshalJSON method (lines 636-654) completely overrides default JSON marshaling behavior. The struct tag is never consulted when a custom marshaler exists.
The actual issue is at lines 645-647, where properties is only included if non-nil. To fix the OpenAI error, modify the custom MarshalJSON to always include the properties field:
Apply this diff to fix the issue:
func (tis ToolArgumentsSchema) MarshalJSON() ([]byte, error) {
m := make(map[string]any)
m["type"] = tis.Type
if tis.Defs != nil {
m["$defs"] = tis.Defs
}
- // Marshal Properties to '{}' rather than `nil` when its length equals zero
- if tis.Properties != nil {
+ // Always include properties field (required by some LLM providers like OpenAI)
+ if tis.Properties == nil {
+ m["properties"] = make(map[string]any)
+ } else {
m["properties"] = tis.Properties
}
if len(tis.Required) > 0 {
m["required"] = tis.Required
}
return json.Marshal(m)
}You can revert the struct tag change on line 628 since it has no effect, or keep it for consistency/documentation purposes.
📝 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.
| Properties map[string]any `json:"properties"` | |
| func (tis ToolArgumentsSchema) MarshalJSON() ([]byte, error) { | |
| m := make(map[string]any) | |
| m["type"] = tis.Type | |
| if tis.Defs != nil { | |
| m["$defs"] = tis.Defs | |
| } | |
| // Always include properties field (required by some LLM providers like OpenAI) | |
| if tis.Properties == nil { | |
| m["properties"] = make(map[string]any) | |
| } else { | |
| m["properties"] = tis.Properties | |
| } | |
| if len(tis.Required) > 0 { | |
| m["required"] = tis.Required | |
| } | |
| return json.Marshal(m) | |
| } |
🤖 Prompt for AI Agents
In mcp/tools.go around lines 628 and 636-654 (and specifically lines 645-647),
the custom MarshalJSON currently omits the "properties" field when properties is
nil, so the struct tag change has no effect; update the MarshalJSON
implementation to always include the "properties" key in the output map
(assigning it the current p.Properties value even if nil) so OpenAI validation
sees the field, and you may revert or keep the struct tag change for
documentation but ensure the custom marshaler always writes the properties
field.
There was a problem hiding this comment.
according to https://github.com/mark3labs/mcp-go/blob/main/mcp/tools.go#L689, properties are always initialized to empty map, this ensures the marshalled json always contain properties field
There was a problem hiding this comment.
@hhxiao You're absolutely right! I missed that important detail. When using NewTool (line 689), Properties is indeed initialized to make(map[string]any), which means it's never nil — it's always an empty map at minimum.
Given this initialization and the existing MarshalJSON logic that checks if tis.Properties != nil, the properties field will always be included in the JSON output for tools created with NewTool, even when empty. This satisfies OpenAI's validation requirement.
So the struct tag change from json:"properties,omitempty" to json:"properties" is actually a no-op in terms of functionality (since the custom marshaler overrides it), but it does serve as good documentation that this field should always be present.
One minor edge case to note: If someone uses NewToolWithRawSchema (line 711) or constructs a Tool struct directly without going through NewTool, the InputSchema.Properties field would be nil, and the current MarshalJSON would omit it. But since the typical/recommended path is using NewTool, this fix correctly addresses the OpenAI validation issue for the common case.
The PR approach is sound! 👍
🧠 Learnings used
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.
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.
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
fix(json): make ToolInputSchema and ToolOutputSchema aliases of ToolArgumentsSchema Previously, ToolInputSchema and ToolOutputSchema were defined as new types, which prevented them from inheriting ToolArgumentsSchema.MarshalJSON. This change converts them into type aliases, ensuring consistent JSON serialization behavior across all schema types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mcp/tools.go (1)
636-654: The MarshalJSON fix from the previous review is still required.The previous review correctly identified that lines 645-647 must be updated to always include the
propertiesfield. Currently, whentis.Propertiesisnil, the field is completely omitted from the JSON output, which triggers OpenAI's validation error.Apply the fix suggested in the previous review:
func (tis ToolArgumentsSchema) MarshalJSON() ([]byte, error) { m := make(map[string]any) m["type"] = tis.Type if tis.Defs != nil { m["$defs"] = tis.Defs } - // Marshal Properties to '{}' rather than `nil` when its length equals zero - if tis.Properties != nil { + // Always include properties field (required by some LLM providers like OpenAI) + if tis.Properties == nil { + m["properties"] = make(map[string]any) + } else { m["properties"] = tis.Properties } if len(tis.Required) > 0 { m["required"] = tis.Required } return json.Marshal(m) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/tools.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.go
🧠 Learnings (7)
📓 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.
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.
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
📚 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.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 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.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.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
mcp/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 **/*.go : For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Applied to files:
mcp/tools.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Applied to files:
mcp/tools.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: coverage
| type ToolInputSchema = ToolArgumentsSchema // For retro-compatibility | ||
| type ToolOutputSchema = ToolArgumentsSchema |
There was a problem hiding this comment.
Type alias change doesn't resolve the OpenAI properties field issue.
Converting these to type aliases is a reasonable refactor for code consistency, but it doesn't fix the stated bug. The OpenAI 400 error occurs because the MarshalJSON method (lines 645-647) omits the properties field when nil.
As noted in the previous review, you must update the MarshalJSON implementation to always include the properties field for object-type schemas, regardless of whether Properties is nil or an empty map.
The PR description states this change "ensures empty properties are preserved," but the type alias change alone doesn't accomplish this. You still need to modify MarshalJSON as suggested in the previous review.
🤖 Prompt for AI Agents
In mcp/tools.go around lines 632-647 (aliases at 632-633 and MarshalJSON
implementation at ~645-647), the type alias refactor didn't address the OpenAI
400 error because MarshalJSON omits the properties field when it's nil; update
MarshalJSON to always include a properties field for schemas whose Type ==
"object" even if Properties is nil by ensuring the encoded JSON contains
"properties": {} (or an empty map) instead of omitting it—e.g., build the output
map/struct explicitly and if Type == "object" set Properties to an empty map
when nil (or remove omitempty for that field) so the properties key is always
present in the serialized JSON.
1 participant
OpenAI will report error if InputSchema of "object" type with no "properties" field existing in its JSON representation.
openai.BadRequestError: Error code: 400 - {'error': {'message': "Invalid schema for function 'getCurrentDate': In context=(), object schema missing properties.", 'type': 'invalid_request_error', 'param': 'tools[0].function.parameters', 'code': 'invalid_function_parameters'}, 'user': '{"appkey": "egai-prd-strategyoffice-020029831-workflow-1746045106141", "session_id": "b95e59c0-a570-4f82-9905-05eff7b35eba-1758281506244515510", "user": "", "prompt_truncate": "yes"}'}
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
Bug Fixes
Refactor