Skip to content

fix: handle []map[string]interface{} content in FormatResponse#67

Open
MekayelAnik wants to merge 1 commit intoFreePeak:mainfrom
MekayelAnik:fix/format-response-content-slice-type
Open

fix: handle []map[string]interface{} content in FormatResponse#67
MekayelAnik wants to merge 1 commit intoFreePeak:mainfrom
MekayelAnik:fix/format-response-content-slice-type

Conversation

@MekayelAnik
Copy link
Copy Markdown

Summary

  • Fix MCP envelope double-wrap in internal/delivery/mcp/response.go FormatResponse
  • Tool handlers in internal/delivery/mcp/tool_types.go and pkg/dbtools/dbtools.go return responses where content is a []map[string]interface{}. The existing type assertion only matched []interface{}, so these returns fell through to return FromString(fmt.Sprintf("%v", response)), nil and the entire MCP envelope was Go-stringified into the outer content[0].text field
  • Accept both []interface{} and []map[string]interface{} via a Go type switch; existing empty-slice and metadata-preservation semantics are unchanged
  • Add TestFormatResponse_ContentSliceTypes that actually asserts the marshaled JSON shape — the existing TestFormatResponse loop discards return values and only verifies absence of panic, which is why this regression went unnoticed

Symptom

Calling any tool against a running server (here query_<db>) yields:

{
  "content": [{
    "type": "text",
    "text": "map[content:[map[text:Results:\n\none\tmsg\n--------------------------------------------------------------------------------\n1\thi\n\nTotal rows: 1 type:text]]]"
  }]
}

Instead of:

{
  "content": [{
    "type": "text",
    "text": "Results:\n\none\tmsg\n--------------------------------------------------------------------------------\n1\thi\n\nTotal rows: 1"
  }]
}

Reproduces against freepeak/db-mcp-server:latest for query_<db>, execute_<db>, schema_<db>, transaction_<db>, performance_<db>, list_databases, and list. The timescaledb_* tools use a separate formatter path and are not affected, which helped isolate the bug to FormatResponse.

Root cause

internal/delivery/mcp/response.go (before this change):

if respMap, ok := response.(map[string]interface{}); ok {
    ...
    if content, exists := respMap["content"]; exists {
        if contentSlice, isSlice := content.([]interface{}); isSlice {
            if len(contentSlice) == 0 {
                return NewResponse(), nil
            }
            return respMap, nil
        }
    }
    ...
}
return FromString(fmt.Sprintf("%v", response)), nil

Handler return shape is map[string]interface{}{"content": []map[string]interface{}{...}} — concrete element type, not []interface{}. The assertion fails, control flows past both inner if blocks, and the catch-all at the bottom Go-stringifies the entire envelope.

Fix

Replace the single-type assertion with a type switch covering both shapes. Inline comment in the source explains why both appear at the boundary.

Test plan

  • go build ./... passes
  • go test ./internal/delivery/mcp/... — 11/11 PASS, including 3 new sub-cases in TestFormatResponse_ContentSliceTypes:
    • content as []interface{} (locks in backward compat)
    • content as []map[string]interface{} (handler return type) (new behavior)
    • content with metadata preserved
  • go test ./... passes for all packages (excluding pkg/db.TestRegressionAllDatabases which requires live MySQL/PostgreSQL on localhost — pre-existing, unrelated to this change)
  • golangci-lint run --timeout=5m ./internal/delivery/mcp/... passes (exit 0, no warnings)
  • End-to-end smoke against a locally-built image: 32/32 tool sub-checks pass; outer text no longer contains map[content:...]. Verified against a 2-DB PostgreSQL+TimescaleDB config.

Notes

The new test case includes an assert.NotContains(out, "map[") defense-in-depth check so this same class of regression cannot silently come back through any other fall-through path in FormatResponse.

Tool handlers in internal/delivery/mcp/tool_types.go and
pkg/dbtools/dbtools.go return responses whose `content` field is a
[]map[string]interface{}. FormatResponse only matched []interface{},
so these returns fell through to the default fmt.Sprintf("%v", response)
branch and the entire MCP envelope was Go-stringified into the outer
text content (visible to clients as
`map[content:[map[text:Results:...]]]`).

Accept both slice types via a type switch. Add a dedicated test that
asserts the marshaled JSON shape (the existing TestFormatResponse loop
discards results and only checks for panics).
Copilot AI review requested due to automatic review settings April 25, 2026 01:57
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes FormatResponse so MCP handler responses that set content as []map[string]interface{} are treated as already-formatted (instead of falling through to the fmt.Sprintf("%v", response) path that Go-stringifies/double-wraps the envelope).

Changes:

  • Update FormatResponse to accept both []interface{} and []map[string]interface{} for the "content" field via a type switch.
  • Add targeted tests that assert the marshaled JSON shape for both content slice types and verify metadata preservation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/delivery/mcp/response.go Expands "content" slice handling to cover []map[string]interface{} and avoid envelope Go-stringification.
internal/delivery/mcp/response_test.go Adds JSON-shape assertions for both content slice types and metadata passthrough.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +144 to 155
name: "map with concrete typed content slice",
input: map[string]interface{}{
"content": []map[string]interface{}{
{"type": "text", "text": "Typed content"},
},
},
err: nil,
expectError: false,
expectedOutput: `{"content":[{"text":"Typed content","type":"text"}]}`,
useMock: false,
},
{
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TestFormatResponse, this new table entry (and the existing ones) sets expectedOutput/err/expectError, but the test body doesn’t assert the output and always calls FormatResponse(tc.input, nil). This makes the new case look covered when it’s effectively a no-op (panic-only). Consider either (a) updating TestFormatResponse to marshal/compare against expectedOutput and pass tc.err, or (b) removing this new case (and/or the unused fields) and relying on TestFormatResponse_ContentSliceTypes for shape assertions.

Suggested change
name: "map with concrete typed content slice",
input: map[string]interface{}{
"content": []map[string]interface{}{
{"type": "text", "text": "Typed content"},
},
},
err: nil,
expectError: false,
expectedOutput: `{"content":[{"text":"Typed content","type":"text"}]}`,
useMock: false,
},
{

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants