mirror of
https://github.com/coder/coder.git
synced 2026-10-31 11:02:50 +08:00
fix: serialize empty MCP tool properties as {} not null for OpenAI (#28380)
## Summary
Fixed a bug where MCP tools with an empty `properties` schema had their
tool definitions sent to OpenAI with `"properties": null`, causing
OpenAI to reject the request.
## Problem
When an MCP server reports a tool with an empty input schema, e.g.:
```json
{
"type": "object",
"properties": {},
"required": []
}
```
Coder was forwarding the tool definition to OpenAI as:
```json
{
"type": "object",
"properties": null
}
```
OpenAI's function-calling API rejects `properties: null`, making these
tools unusable with OpenAI models.
## Root cause
The empty `properties` object collapsed to Go `nil` at two layers:
1. `agent/x/agentmcp/manager.go` `toolInputSchemaMap` stripped the
present-but-empty `"properties": {}` (guarded by `len(properties) > 0`),
so the wire copy lost it entirely.
2. `coderd/x/chatd/chatloop/chatloop.go` `buildToolDefinitions` embedded
`info.Parameters` directly into the input schema. When nil,
`encoding/json` marshals it as `null`.
## Fix
- `buildToolDefinitions` now substitutes an empty object when
`Parameters` is nil, mirroring the existing `required` guard (the sink,
protects all tool sources).
- `toolInputSchemaMap` now preserves an empty `properties` object
instead of dropping it (the upstream producer).
## Tests
Added regression tests (failing before, passing after) covering both
layers and asserting the schema serializes to
`{"type":"object","properties":{}}`:
- `TestBuildToolDefinitionsNilPropertiesBecomesEmptyObject`
- `TestToolInputSchemaMapPreservesEmptyProperties`
-
`TestWorkspaceMCPToolInfosFromResources/EmptyPropertiesYieldsEmptySchema`
---
*This PR was generated by Coder Agents.*
This commit is contained in:
@@ -1628,9 +1628,16 @@ func buildToolDefinitions(tools []fantasy.AgentTool, activeTools []string, provi
|
||||
continue
|
||||
}
|
||||
|
||||
// Substitute an empty object for nil properties so that a tool
|
||||
// with no parameters never serializes "properties" to null,
|
||||
// which OpenAI rejects.
|
||||
properties := info.Parameters
|
||||
if properties == nil {
|
||||
properties = map[string]any{}
|
||||
}
|
||||
inputSchema := map[string]any{
|
||||
"type": "object",
|
||||
"properties": info.Parameters,
|
||||
"properties": properties,
|
||||
}
|
||||
// Only include "required" when non-empty so that a nil slice
|
||||
// never serializes to null, which OpenAI rejects.
|
||||
|
||||
@@ -2,6 +2,7 @@ package chatloop
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"iter"
|
||||
"testing"
|
||||
|
||||
@@ -81,6 +82,52 @@ func TestProcessStepStreamPersistsRedactedThinkingOnEnd(t *testing.T) {
|
||||
require.Equal(t, "redacted-payload", metadata.RedactedData)
|
||||
}
|
||||
|
||||
// staticParametersTool returns a fixed ToolInfo, letting a test control
|
||||
// Parameters directly. fantasy.NewAgentTool always generates a non-nil
|
||||
// Parameters map, so it cannot reproduce the nil Parameters that MCP tool
|
||||
// wrappers report for a schema with an empty "properties" object.
|
||||
type staticParametersTool struct {
|
||||
fantasy.AgentTool
|
||||
info fantasy.ToolInfo
|
||||
}
|
||||
|
||||
func (t staticParametersTool) Info() fantasy.ToolInfo { return t.info }
|
||||
|
||||
func (staticParametersTool) ProviderOptions() fantasy.ProviderOptions { return nil }
|
||||
|
||||
// TestBuildToolDefinitionsNilPropertiesBecomesEmptyObject verifies that a
|
||||
// tool whose input schema has no properties (for example an MCP tool
|
||||
// reporting {"type": "object", "properties": {}}) still serializes
|
||||
// "properties" as an empty JSON object. A nil Parameters map would serialize
|
||||
// to null, which OpenAI rejects with "Invalid schema for function ... None is
|
||||
// not of type 'object'".
|
||||
func TestBuildToolDefinitionsNilPropertiesBecomesEmptyObject(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tool := staticParametersTool{
|
||||
info: fantasy.ToolInfo{
|
||||
Name: "document_graphql_schema",
|
||||
Description: "Run a GraphQL query",
|
||||
Parameters: nil,
|
||||
},
|
||||
}
|
||||
|
||||
defs := buildToolDefinitions([]fantasy.AgentTool{tool}, nil, nil)
|
||||
require.Len(t, defs, 1)
|
||||
|
||||
ft, ok := defs[0].(fantasy.FunctionTool)
|
||||
require.True(t, ok, "expected a fantasy.FunctionTool")
|
||||
|
||||
properties, ok := ft.InputSchema["properties"].(map[string]any)
|
||||
require.True(t, ok, "properties must be a map, got %T", ft.InputSchema["properties"])
|
||||
require.NotNil(t, properties, "properties must not be nil")
|
||||
|
||||
// Verify it serializes to {} not null.
|
||||
bs, err := json.Marshal(ft.InputSchema)
|
||||
require.NoError(t, err)
|
||||
require.JSONEq(t, `{"type":"object","properties":{}}`, string(bs))
|
||||
}
|
||||
|
||||
func TestFlushActiveStatePreservesEmptySignedReasoning(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -735,6 +735,31 @@ func TestWorkspaceMCPToolInfosFromResources(t *testing.T) {
|
||||
require.Equal(t, "playwright__navigate", infos[0].Name)
|
||||
})
|
||||
|
||||
t.Run("EmptyPropertiesYieldsEmptySchema", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// An MCP tool reporting {"type": "object", "properties": {}} must
|
||||
// produce a non-nil empty Schema. A nil Schema serializes to JSON
|
||||
// null downstream, which OpenAI rejects.
|
||||
schema := mustStruct(t, map[string]any{
|
||||
"type": "object",
|
||||
"properties": map[string]any{},
|
||||
})
|
||||
resources := []database.ChatContextResource{
|
||||
mcpServerResource(t, "github", &agentproto.MCPServerBody{
|
||||
ServerName: "github",
|
||||
Tools: []*agentproto.MCPTool{
|
||||
{Name: "document_graphql_schema", Description: "Run a GraphQL query", InputSchema: schema},
|
||||
},
|
||||
}, database.WorkspaceAgentContextResourceStatusOk),
|
||||
}
|
||||
|
||||
infos := workspaceMCPToolInfosFromResources(resources)
|
||||
require.Len(t, infos, 1)
|
||||
require.NotNil(t, infos[0].Schema, "Schema must not be nil for an empty properties object")
|
||||
require.Empty(t, infos[0].Schema)
|
||||
})
|
||||
|
||||
t.Run("NoMCPServersYieldsNil", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user