Extract shared MCP content-slice normalization#7325
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes MCP "content" slice normalization into mcpresult.NormalizeContentItems to remove duplicated handling of []interface{} vs []map[string]interface{} across MCP result conversion and middleware, while keeping each call site’s existing invalid-item semantics.
Changes:
- Adds
mcpresult.NormalizeContentItems(contentVal interface{})to normalize"content"into[]map[string]interface{}. - Refactors MCP result conversion, text extraction, and middleware fast-path/rewrite logic to use the shared helper.
- Adds unit tests for the shared helper and a middleware fast-path test for typed
[]map[string]interface{}content slices.
Show a summary per file
| File | Description |
|---|---|
| internal/middleware/jqschema.go | Uses shared content-slice normalization for envelope rewrite and fast-path checks. |
| internal/middleware/jqschema_test.go | Adds coverage ensuring fast-path behavior works with typed []map[string]interface{} content. |
| internal/mcpresult/text_content.go | Replaces local content normalization with NormalizeContentItems. |
| internal/mcpresult/content_items.go | Introduces the shared NormalizeContentItems helper. |
| internal/mcpresult/content_items_test.go | Adds focused unit tests for the new normalization helper. |
| internal/mcp/tool_result.go | Refactors result conversion to use shared normalization while preserving strict rejection semantics. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
| items, ok := mcpresult.NormalizeContentItems(contentVal) | ||
| if !ok { | ||
| // content field exists but is not a recognizable slice type — wrap the whole map as text. | ||
| return marshalValueToTextContentResult(m) | ||
| } | ||
| if rawItems, ok := contentVal.([]interface{}); ok && len(items) != len(rawItems) { | ||
| for i, item := range rawItems { | ||
| if _, ok := item.(map[string]interface{}); !ok { | ||
| return nil, fmt.Errorf("content item %d: expected map, got %T", i, item) | ||
| } |
…e-content-slice-normalization # Conflicts: # internal/middleware/jqschema.go
Merge conflicts resolved in 7781dd1. The only conflict was in |
|
@copilot address review feedback |
The MCP
"content"field normalization logic was duplicated acrossmcp,mcpresult, and middleware, with behavior split between[]interface{}and[]map[string]interface{}inputs. This change centralizes that slice normalization while preserving each caller’s existing handling of invalid content items.Shared normalization helper
mcpresult.NormalizeContentItems(contentVal interface{}) ([]map[string]interface{}, bool)as the single normalization path for MCP content slices.[]interface{}) and typed helper-produced slices ([]map[string]interface{}).Call site refactor
internal/mcp/tool_result.gointernal/mcpresult/text_content.gointernal/middleware/jqschema.goConvertToCallToolResultstill rejects non-map items in[]interface{}ExtractTextContentstill skips non-map itemsCoverage additions
[]map[string]interface{}fast path, which was one of the duplicated branches called out in the issue.Example