From f4eaf04fd3190ff1c808ff65faf8fd0b665633aa Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 09:04:03 +0300 Subject: [PATCH 1/2] fix(mcp): serialize empty retrieve_tools result as [] instead of null A nil tools slice marshals as JSON null, crashing strict MCP clients that iterate the array ('NoneType' object is not iterable). Initialize the slice so zero-match searches (including results fully removed by the spec-035 annotation filter) always emit "tools": []. Fixes #953 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W2f2uxUUtENehyZoiVLQKN --- internal/server/mcp.go | 4 +++- internal/server/mcp_empty_results_test.go | 26 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 internal/server/mcp_empty_results_test.go diff --git a/internal/server/mcp.go b/internal/server/mcp.go index c0dbcd53..1473d2d5 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -1600,7 +1600,9 @@ func (p *MCPProxyServer) handleRetrieveToolsWithMode(ctx context.Context, reques // ranked order of `results` is already final here — the mode selects // serialization only (FR-007). entryOpts := toolEntryOpts{includeStats: includeStats} - var mcpTools []map[string]interface{} + // Issue #953: must be non-nil so zero matches serialize as [] — strict MCP + // clients crash iterating a null tools array. + mcpTools := make([]map[string]interface{}, 0, len(results)) for _, result := range results { mcpTools = append(mcpTools, p.buildToolEntry(result, responseMode, entryOpts)) } diff --git a/internal/server/mcp_empty_results_test.go b/internal/server/mcp_empty_results_test.go new file mode 100644 index 00000000..c3da478a --- /dev/null +++ b/internal/server/mcp_empty_results_test.go @@ -0,0 +1,26 @@ +package server + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #953: a retrieve_tools search with zero matches must serialize the +// tools array as [] — never null. Strict MCP clients iterate over the array +// and crash on null (e.g. Python's `'NoneType' object is not iterable`). +func TestRetrieveTools_EmptyResultSerializesEmptyArray(t *testing.T) { + proxy := createTestMCPProxyServer(t) + seedEntryBuilderFixture(t, proxy) + + resp, raw := callRetrieve(t, proxy, map[string]interface{}{ + "query": "zzz-no-such-tool-anywhere", "limit": float64(10), + }) + + require.Equal(t, 0, resp.Total, "fixture must not match the nonsense query") + assert.NotContains(t, raw, `"tools":null`, "nil slice must not serialize as null") + assert.True(t, strings.Contains(raw, `"tools":[]`), + "empty result must serialize tools as an empty array, got: %s", raw) +} From a991bf4c1d274ab3d2065c3ee8cba6951c24711a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 09:39:27 +0300 Subject: [PATCH 2/2] fix(mcp): eliminate remaining null-array serializations (review round 1) Cross-model review found sibling sites with the same nil-slice defect as issue #953: read_cache.records, quarantine_security servers, inspect_quarantined tools, search_servers servers, and retrieve_tools usage_summary.top_tools. All now serialize as [] when empty. The spec-085 golden fixture is updated for the intentional top_tools change (null -> []); the retrieve_tools regression test now asserts the wire type via json.RawMessage instead of substring matching. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W2f2uxUUtENehyZoiVLQKN --- internal/cache/empty_records_test.go | 60 +++++++++++++++++++ internal/cache/manager.go | 4 +- internal/registries/search.go | 8 +++ internal/registries/search_empty_test.go | 43 +++++++++++++ internal/server/mcp.go | 4 +- internal/server/mcp_empty_results_test.go | 34 +++++++++-- .../testdata/retrieve_full_stats.golden.json | 2 +- internal/storage/empty_slices_test.go | 46 ++++++++++++++ internal/storage/manager.go | 8 ++- 9 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 internal/cache/empty_records_test.go create mode 100644 internal/registries/search_empty_test.go create mode 100644 internal/storage/empty_slices_test.go diff --git a/internal/cache/empty_records_test.go b/internal/cache/empty_records_test.go new file mode 100644 index 00000000..92eb10a4 --- /dev/null +++ b/internal/cache/empty_records_test.go @@ -0,0 +1,60 @@ +package cache + +import ( + "testing" + + "go.uber.org/zap" +) + +// Issue #953 follow-up: an empty (or fully paginated-past) record set must +// yield a non-nil Records slice so read_cache serializes "records": [] — +// never null — for strict MCP clients that iterate the array. +func TestGetRecords_EmptyContentReturnsNonNilRecords(t *testing.T) { + db := setupTestDB(t) + defer db.Close() + + manager, err := NewManager(db, zap.NewNop()) + if err != nil { + t.Fatalf("Failed to create cache manager: %v", err) + } + defer manager.Close() + + if err := manager.Store("empty-key", "test_tool", nil, "[]", "", 0); err != nil { + t.Fatalf("Failed to store record: %v", err) + } + + resp, err := manager.GetRecords("empty-key", 0, 10) + if err != nil { + t.Fatalf("GetRecords failed: %v", err) + } + if resp.Records == nil { + t.Fatal("Records must be a non-nil slice so it serializes as [], not null") + } + if len(resp.Records) != 0 { + t.Fatalf("expected 0 records, got %d", len(resp.Records)) + } +} + +// Paginating past the end of a non-empty set must also stay non-nil. +func TestGetRecords_OffsetPastEndReturnsNonNilRecords(t *testing.T) { + db := setupTestDB(t) + defer db.Close() + + manager, err := NewManager(db, zap.NewNop()) + if err != nil { + t.Fatalf("Failed to create cache manager: %v", err) + } + defer manager.Close() + + if err := manager.Store("two-key", "test_tool", nil, `["a","b"]`, "", 2); err != nil { + t.Fatalf("Failed to store record: %v", err) + } + + resp, err := manager.GetRecords("two-key", 5, 10) + if err != nil { + t.Fatalf("GetRecords failed: %v", err) + } + if resp.Records == nil { + t.Fatal("Records must be a non-nil slice so it serializes as [], not null") + } +} diff --git a/internal/cache/manager.go b/internal/cache/manager.go index 2f00173d..18b7d119 100644 --- a/internal/cache/manager.go +++ b/internal/cache/manager.go @@ -219,7 +219,9 @@ func (m *Manager) GetRecords(key string, offset, limit int) (*ReadCacheResponse, end = totalRecords } - var paginatedRecords []interface{} + // Non-nil so an empty page serializes as "records": [] — never null + // (issue #953: strict MCP clients crash iterating a null array). + paginatedRecords := make([]interface{}, 0) if offset < totalRecords { paginatedRecords = records[offset:end] } diff --git a/internal/registries/search.go b/internal/registries/search.go index f9928e66..189e5f96 100644 --- a/internal/registries/search.go +++ b/internal/registries/search.go @@ -103,6 +103,14 @@ func SearchServers(ctx context.Context, registryID, tag, query string, limit int filtered[i].Registry = reg.Name } + // Non-nil so an empty result serializes as "servers": [] — never null + // (issue #953: strict MCP clients crash iterating a null array). An empty + // query skips filterServers' allocation, so a nil fetch result would + // otherwise flow straight through. + if filtered == nil { + filtered = []ServerEntry{} + } + return filtered, nil } diff --git a/internal/registries/search_empty_test.go b/internal/registries/search_empty_test.go new file mode 100644 index 00000000..ee224a72 --- /dev/null +++ b/internal/registries/search_empty_test.go @@ -0,0 +1,43 @@ +package registries + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" +) + +// Issue #953 follow-up: a registry with zero servers must produce a non-nil +// slice so search_servers serializes "servers": [] — never null — for strict +// MCP clients that iterate the array. With an empty query filterServers +// returns the fetched slice untouched, so the nil has to be stopped at the +// SearchServers boundary. +func TestSearchServers_EmptyRegistryReturnsNonNil(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"servers": []}`)) + })) + defer server.Close() + + originalList := registryList + registryList = []RegistryEntry{ + { + ID: "test-empty", + Name: "Test Empty Registry", + ServersURL: server.URL, + Protocol: "modelcontextprotocol/registry", + }, + } + defer func() { registryList = originalList }() + + servers, err := SearchServers(context.Background(), "test-empty", "", "", 10, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if servers == nil { + t.Fatal("servers must be a non-nil slice so it serializes as [], not null") + } + if len(servers) != 0 { + t.Fatalf("expected 0 servers, got %d", len(servers)) + } +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 1473d2d5..e1f956c1 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -3780,7 +3780,9 @@ func (p *MCPProxyServer) handleInspectQuarantinedTools(ctx context.Context, requ return mcp.NewToolResultError(reason), nil } - var toolsAnalysis []map[string]interface{} + // Non-nil so a tool-less server serializes as "tools": [] — never null + // (issue #953: strict MCP clients crash iterating a null array). + toolsAnalysis := make([]map[string]interface{}, 0) // REQUEST TEMPORARY CONNECTION EXEMPTION FOR INSPECTION p.logger.Warn("⚠️ Requesting temporary connection exemption for quarantined server inspection", diff --git a/internal/server/mcp_empty_results_test.go b/internal/server/mcp_empty_results_test.go index c3da478a..2f3848c6 100644 --- a/internal/server/mcp_empty_results_test.go +++ b/internal/server/mcp_empty_results_test.go @@ -1,13 +1,26 @@ package server import ( - "strings" + "context" + "encoding/json" "testing" - "github.com/stretchr/testify/assert" + "github.com/mark3labs/mcp-go/mcp" "github.com/stretchr/testify/require" ) +// assertJSONFieldIsEmptyArray decodes raw and asserts field is exactly [] on +// the wire — the strict-client contract from issue #953 (null crashes clients +// that iterate the array). +func assertJSONFieldIsEmptyArray(t *testing.T, raw, field string) { + t.Helper() + var payload map[string]json.RawMessage + require.NoError(t, json.Unmarshal([]byte(raw), &payload)) + require.Contains(t, payload, field) + require.JSONEq(t, `[]`, string(payload[field]), + "%q must serialize as an empty array, not %s", field, payload[field]) +} + // Issue #953: a retrieve_tools search with zero matches must serialize the // tools array as [] — never null. Strict MCP clients iterate over the array // and crash on null (e.g. Python's `'NoneType' object is not iterable`). @@ -20,7 +33,18 @@ func TestRetrieveTools_EmptyResultSerializesEmptyArray(t *testing.T) { }) require.Equal(t, 0, resp.Total, "fixture must not match the nonsense query") - assert.NotContains(t, raw, `"tools":null`, "nil slice must not serialize as null") - assert.True(t, strings.Contains(raw, `"tools":[]`), - "empty result must serialize tools as an empty array, got: %s", raw) + assertJSONFieldIsEmptyArray(t, raw, "tools") +} + +// Same contract for quarantine_security list_quarantined with no quarantined +// servers: "servers" must be [] on the wire, never null. +func TestListQuarantined_EmptySerializesEmptyArray(t *testing.T) { + proxy := createTestMCPProxyServer(t) + + result, err := proxy.handleListQuarantinedUpstreams(context.Background()) + require.NoError(t, err) + require.False(t, result.IsError) + + raw := result.Content[0].(mcp.TextContent).Text + assertJSONFieldIsEmptyArray(t, raw, "servers") } diff --git a/internal/server/testdata/retrieve_full_stats.golden.json b/internal/server/testdata/retrieve_full_stats.golden.json index a86fd546..afac70bf 100644 --- a/internal/server/testdata/retrieve_full_stats.golden.json +++ b/internal/server/testdata/retrieve_full_stats.golden.json @@ -1 +1 @@ -{"query":"manage","tools":[{"call_with":"call_tool_read","description":"Get repository metadata to manage projects.","inputSchema":{"properties":{},"type":"object"},"name":"github:get_repo","score":0.011312138508548623,"server":"github"},{"call_with":"call_tool_read","description":"List issues to manage a repository backlog.","inputSchema":{"properties":{"repo":{"type":"string"},"state":{"enum":["open","closed","all"]}},"required":["repo"],"type":"object"},"name":"github:list_issues","score":0.008243798662285959,"server":"github"},{"call_with":"call_tool_read","description":"Search cities to manage location lookups across regions worldwide.","inputSchema":{"properties":{"id":{"type":["string","integer"]},"q":{"type":"string"}},"required":["q"],"type":"object"},"name":"weather:search_city","score":0.007666460133457465,"server":"weather"},{"call_with":"call_tool_read","description":"Get a weather forecast to manage travel plans.","inputSchema":{"properties":{"days":{"type":"integer"},"location":{"properties":{"lat":{"type":"number"},"lon":{"type":"number"}},"type":"object"}},"required":["location"],"type":"object"},"name":"weather:get_forecast","score":0.007056919206727505,"server":"weather"},{"call_with":"call_tool_read","description":"Create an issue to manage work. Supports labels and assignees.","inputSchema":{"properties":{"body":{"type":"string"},"labels":{"items":{"type":"string"},"type":"array"},"title":{"type":"string"},"ttl":{"default":3600,"type":"integer"}},"required":["title"],"type":"object"},"name":"github:create_issue","score":0.00646672399372599,"server":"github"}],"total":5,"usage_instructions":"TOOL SELECTION GUIDE: Check the 'call_with' field for each tool, then use the matching tool variant. DECISION RULES BY TOOL NAME: (1) READ (call_tool_read): search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. DEFAULT choice when unsure. (2) WRITE (call_tool_write): create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit, assign, configure, enable, register, subscribe, publish, move, copy, rename, merge. (3) DESTRUCTIVE (call_tool_destructive): delete, remove, drop, revoke, disable, destroy, purge, reset, clear, unsubscribe, cancel, terminate, close, archive, ban, block, disconnect, kill, wipe, truncate, force, hard. INTENT TRACKING: Always provide intent_reason (why you're calling this tool) and intent_data_sensitivity (public/internal/private/unknown) to enable activity auditing.","usage_summary":{"top_tools":null}} +{"query":"manage","tools":[{"call_with":"call_tool_read","description":"Get repository metadata to manage projects.","inputSchema":{"properties":{},"type":"object"},"name":"github:get_repo","score":0.011312138508548623,"server":"github"},{"call_with":"call_tool_read","description":"List issues to manage a repository backlog.","inputSchema":{"properties":{"repo":{"type":"string"},"state":{"enum":["open","closed","all"]}},"required":["repo"],"type":"object"},"name":"github:list_issues","score":0.008243798662285959,"server":"github"},{"call_with":"call_tool_read","description":"Search cities to manage location lookups across regions worldwide.","inputSchema":{"properties":{"id":{"type":["string","integer"]},"q":{"type":"string"}},"required":["q"],"type":"object"},"name":"weather:search_city","score":0.007666460133457465,"server":"weather"},{"call_with":"call_tool_read","description":"Get a weather forecast to manage travel plans.","inputSchema":{"properties":{"days":{"type":"integer"},"location":{"properties":{"lat":{"type":"number"},"lon":{"type":"number"}},"type":"object"}},"required":["location"],"type":"object"},"name":"weather:get_forecast","score":0.007056919206727505,"server":"weather"},{"call_with":"call_tool_read","description":"Create an issue to manage work. Supports labels and assignees.","inputSchema":{"properties":{"body":{"type":"string"},"labels":{"items":{"type":"string"},"type":"array"},"title":{"type":"string"},"ttl":{"default":3600,"type":"integer"}},"required":["title"],"type":"object"},"name":"github:create_issue","score":0.00646672399372599,"server":"github"}],"total":5,"usage_instructions":"TOOL SELECTION GUIDE: Check the 'call_with' field for each tool, then use the matching tool variant. DECISION RULES BY TOOL NAME: (1) READ (call_tool_read): search, query, list, get, fetch, find, check, view, read, show, describe, lookup, retrieve, browse, explore, discover, scan, inspect, analyze, examine, validate, verify. DEFAULT choice when unsure. (2) WRITE (call_tool_write): create, update, modify, add, set, send, edit, change, write, post, put, patch, insert, upload, submit, assign, configure, enable, register, subscribe, publish, move, copy, rename, merge. (3) DESTRUCTIVE (call_tool_destructive): delete, remove, drop, revoke, disable, destroy, purge, reset, clear, unsubscribe, cancel, terminate, close, archive, ban, block, disconnect, kill, wipe, truncate, force, hard. INTENT TRACKING: Always provide intent_reason (why you're calling this tool) and intent_data_sensitivity (public/internal/private/unknown) to enable activity auditing.","usage_summary":{"top_tools":[]}} diff --git a/internal/storage/empty_slices_test.go b/internal/storage/empty_slices_test.go new file mode 100644 index 00000000..29cccb3e --- /dev/null +++ b/internal/storage/empty_slices_test.go @@ -0,0 +1,46 @@ +package storage + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +func setupTestStorageForEmptySlices(t *testing.T) *Manager { + t.Helper() + + tmpDir, err := os.MkdirTemp("", "empty_slices_test_*") + require.NoError(t, err) + + manager, err := NewManager(tmpDir, zap.NewNop().Sugar()) + require.NoError(t, err) + + t.Cleanup(func() { + manager.Close() + os.RemoveAll(tmpDir) + }) + return manager +} + +// Issue #953 follow-up: MCP responses built from these slices must serialize +// as [] — never null — for strict clients that iterate the arrays. + +func TestListQuarantinedUpstreamServers_EmptyReturnsNonNil(t *testing.T) { + manager := setupTestStorageForEmptySlices(t) + + servers, err := manager.ListQuarantinedUpstreamServers() + require.NoError(t, err) + require.NotNil(t, servers, "quarantined-server list must be non-nil so it serializes as [], not null") + require.Empty(t, servers) +} + +func TestGetToolStats_NoStatsReturnsNonNil(t *testing.T) { + manager := setupTestStorageForEmptySlices(t) + + stats, err := manager.GetToolStats(10) + require.NoError(t, err) + require.NotNil(t, stats, "tool stats must be non-nil so usage_summary.top_tools serializes as [], not null") + require.Empty(t, stats) +} diff --git a/internal/storage/manager.go b/internal/storage/manager.go index 95c7727a..77c2d58c 100644 --- a/internal/storage/manager.go +++ b/internal/storage/manager.go @@ -242,7 +242,9 @@ func (m *Manager) ListQuarantinedUpstreamServers() ([]*config.ServerConfig, erro m.logger.Debugw("Retrieved all upstream records for quarantine filtering", "total_records", len(records)) - var quarantinedServers []*config.ServerConfig + // Non-nil so an empty list serializes as "servers": [] — never null + // (issue #953: strict MCP clients crash iterating a null array). + quarantinedServers := make([]*config.ServerConfig, 0) for _, record := range records { m.logger.Debugw("Checking server quarantine status", "server", record.Name, @@ -819,7 +821,9 @@ func (m *Manager) GetToolStats(topN int) ([]map[string]interface{}, error) { return nil, err } - var result []map[string]interface{} + // Non-nil so usage_summary.top_tools serializes as [] — never null + // (issue #953: strict MCP clients crash iterating a null array). + result := make([]map[string]interface{}, 0, len(stats.TopTools)) for _, tool := range stats.TopTools { result = append(result, map[string]interface{}{ "tool_name": tool.ToolName,