fix(mcp): normalize stringified container arguments (#1319)

Co-authored-by: ntduc <ntduc@cpp.ai.vn>

internal/mcp/bridge_tool.go

@@ -39,9 +39,9 @@ func argMapKeys(m map[string]any) string {
 // safe reconnection without data races.
 type BridgeTool struct {
 	serverName        string
-	serverID          uuid.UUID    // MCP server ID (for grant recheck)
-	toolName          string       // original MCP tool name
-	registeredName    string       // may include prefix: "{prefix}__{toolName}"
+	serverID          uuid.UUID // MCP server ID (for grant recheck)
+	toolName          string    // original MCP tool name
+	registeredName    string    // may include prefix: "{prefix}__{toolName}"
 	description       string
 	descriptionSuffix string         // admin-authored hints appended to description (see WithHints)
 	inputSchema       map[string]any // JSON Schema for parameters
@@ -224,6 +224,22 @@ func (t *BridgeTool) Execute(ctx context.Context, args map[string]any) *tools.Re
 	// instead of omitting them, causing MCP servers to reject invalid values
 	// (e.g. empty string for UUID fields).
 	cleanedArgs := t.stripEmptyOptionalArgs(args)
+	var coercedPaths []string
+	var normalizeErr error
+	cleanedArgs, coercedPaths, normalizeErr = t.normalizeArgsForSchema(cleanedArgs)
+	if normalizeErr != nil {
+		return tools.ErrorResult(fmt.Sprintf(
+			"MCP tool %q argument error: %v. For object or array parameters, pass native JSON values, not stringified JSON.",
+			t.registeredName, normalizeErr))
+	}
+	if len(coercedPaths) > 0 {
+		slog.Warn("mcp.tool.args.coerced",
+			"server", t.serverName,
+			"tool", t.registeredName,
+			"user_id", store.UserIDFromContext(ctx),
+			"agent_id", store.AgentIDFromContext(ctx),
+			"paths", strings.Join(coercedPaths, ","))
+	}
 
 	req := mcpgo.CallToolRequest{}
 	req.Params.Name = t.toolName

internal/mcp/bridge_tool_arg_normalization.go

@@ -0,0 +1,123 @@
+package mcp
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+)
+
+func (t *BridgeTool) normalizeArgsForSchema(args map[string]any) (map[string]any, []string, error) {
+	if len(args) == 0 {
+		return args, nil, nil
+	}
+
+	normalized, coercedPaths, err := normalizeValueForSchema(args, t.inputSchema, "$")
+	if err != nil {
+		return nil, nil, err
+	}
+	normalizedArgs, ok := normalized.(map[string]any)
+	if !ok {
+		return nil, nil, fmt.Errorf("arguments must be object, got %s", valueKind(normalized))
+	}
+	return normalizedArgs, coercedPaths, nil
+}
+
+func normalizeValueForSchema(value any, schema map[string]any, path string) (any, []string, error) {
+	if schema == nil {
+		return value, nil, nil
+	}
+
+	types := schemaTypeSet(schema)
+	expectsContainer := (types["object"] || types["array"]) && !types["string"]
+	if s, ok := value.(string); ok && expectsContainer {
+		return normalizeStringContainer(s, schema, types, path)
+	}
+
+	if obj, ok := value.(map[string]any); ok {
+		return normalizeObjectValue(obj, schema, path)
+	}
+	if arr, ok := value.([]any); ok {
+		return normalizeArrayValue(arr, schema, path)
+	}
+	return value, nil, nil
+}
+
+func normalizeStringContainer(raw string, schema map[string]any, types map[string]bool, path string) (any, []string, error) {
+	trimmed := strings.TrimSpace(raw)
+	if trimmed == "" {
+		return nil, nil, fmt.Errorf("%s must be %s, got empty string", path, expectedContainerLabel(types))
+	}
+
+	var decoded any
+	if err := json.Unmarshal([]byte(trimmed), &decoded); err != nil {
+		return nil, nil, fmt.Errorf("%s must be %s, got JSON string that cannot be parsed: pass a native value instead of a stringified JSON value", path, expectedContainerLabel(types))
+	}
+
+	switch v := decoded.(type) {
+	case map[string]any:
+		if !types["object"] {
+			return nil, nil, fmt.Errorf("%s must be %s, got JSON string containing object", path, expectedContainerLabel(types))
+		}
+		normalized, nestedPaths, err := normalizeObjectValue(v, schema, path)
+		if err != nil {
+			return nil, nil, err
+		}
+		return normalized, prependPath(path, nestedPaths), nil
+	case []any:
+		if !types["array"] {
+			return nil, nil, fmt.Errorf("%s must be %s, got JSON string containing array", path, expectedContainerLabel(types))
+		}
+		normalized, nestedPaths, err := normalizeArrayValue(v, schema, path)
+		if err != nil {
+			return nil, nil, err
+		}
+		return normalized, prependPath(path, nestedPaths), nil
+	default:
+		return nil, nil, fmt.Errorf("%s must be %s, got JSON string containing %s", path, expectedContainerLabel(types), valueKind(decoded))
+	}
+}
+
+func normalizeObjectValue(obj map[string]any, schema map[string]any, path string) (map[string]any, []string, error) {
+	props, _ := schema["properties"].(map[string]any)
+	if len(props) == 0 {
+		return obj, nil, nil
+	}
+
+	normalized := make(map[string]any, len(obj))
+	var coercedPaths []string
+	for key, value := range obj {
+		propSchema, _ := props[key].(map[string]any)
+		if propSchema == nil {
+			normalized[key] = value
+			continue
+		}
+		nextPath := joinSchemaPath(path, key)
+		nextValue, nestedPaths, err := normalizeValueForSchema(value, propSchema, nextPath)
+		if err != nil {
+			return nil, nil, err
+		}
+		normalized[key] = nextValue
+		coercedPaths = append(coercedPaths, nestedPaths...)
+	}
+	return normalized, coercedPaths, nil
+}
+
+func normalizeArrayValue(arr []any, schema map[string]any, path string) ([]any, []string, error) {
+	itemSchema, _ := schema["items"].(map[string]any)
+	if itemSchema == nil {
+		return arr, nil, nil
+	}
+
+	normalized := make([]any, len(arr))
+	var coercedPaths []string
+	for i, value := range arr {
+		nextPath := fmt.Sprintf("%s[%d]", path, i)
+		nextValue, nestedPaths, err := normalizeValueForSchema(value, itemSchema, nextPath)
+		if err != nil {
+			return nil, nil, err
+		}
+		normalized[i] = nextValue
+		coercedPaths = append(coercedPaths, nestedPaths...)
+	}
+	return normalized, coercedPaths, nil
+}

internal/mcp/bridge_tool_schema_helpers.go

@@ -0,0 +1,93 @@
+package mcp
+
+import "fmt"
+
+func schemaTypeSet(schema map[string]any) map[string]bool {
+	types := make(map[string]bool)
+	addSchemaTypes(schema, types)
+	if len(types) == 0 {
+		if _, ok := schema["properties"]; ok {
+			types["object"] = true
+		}
+		if _, ok := schema["items"]; ok {
+			types["array"] = true
+		}
+	}
+	return types
+}
+
+func addSchemaTypes(schema map[string]any, types map[string]bool) {
+	addTypeValue(schema["type"], types)
+	for _, key := range []string{"anyOf", "oneOf", "allOf"} {
+		alternatives, _ := schema[key].([]any)
+		for _, alternative := range alternatives {
+			altSchema, _ := alternative.(map[string]any)
+			if altSchema != nil {
+				addSchemaTypes(altSchema, types)
+			}
+		}
+	}
+}
+
+func addTypeValue(value any, types map[string]bool) {
+	switch v := value.(type) {
+	case string:
+		if v != "" {
+			types[v] = true
+		}
+	case []any:
+		for _, item := range v {
+			addTypeValue(item, types)
+		}
+	case []string:
+		for _, item := range v {
+			addTypeValue(item, types)
+		}
+	}
+}
+
+func expectedContainerLabel(types map[string]bool) string {
+	switch {
+	case types["object"] && types["array"]:
+		return "object or array"
+	case types["object"]:
+		return "object"
+	case types["array"]:
+		return "array"
+	default:
+		return "non-string JSON container"
+	}
+}
+
+func prependPath(path string, nestedPaths []string) []string {
+	if len(nestedPaths) > 0 {
+		return append([]string{path}, nestedPaths...)
+	}
+	return []string{path}
+}
+
+func joinSchemaPath(path, key string) string {
+	if path == "" || path == "$" {
+		return "$." + key
+	}
+	return path + "." + key
+}
+
+func valueKind(value any) string {
+	switch value.(type) {
+	case nil:
+		return "null"
+	case map[string]any:
+		return "object"
+	case []any:
+		return "array"
+	case string:
+		return "string"
+	case bool:
+		return "boolean"
+	case float64, float32, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64:
+		return "number"
+	default:
+		return fmt.Sprintf("%T", value)
+	}
+}

internal/mcp/bridge_tool_test.go

@@ -1,6 +1,7 @@
 package mcp
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/google/uuid"
@@ -235,10 +236,10 @@ func TestStripEmptyOptionalArgs(t *testing.T) {
 
 	args := map[string]any{
 		"url":      "https://example.com",
-		"api_key":  "optional",    // placeholder → strip
-		"timeout":  nil,           // nil → strip
-		"debug":    true,          // real boolean → keep
-		"keywords": "",            // empty string for string-typed → keep
+		"api_key":  "optional", // placeholder → strip
+		"timeout":  nil,        // nil → strip
+		"debug":    true,       // real boolean → keep
+		"keywords": "",         // empty string for string-typed → keep
 	}
 
 	cleaned := bt.stripEmptyOptionalArgs(args)
@@ -287,6 +288,177 @@ func TestStripEmptyOptionalArgs_EmptyStringNonString(t *testing.T) {
 	}
 }
 
+func TestNormalizeArgsForSchema_CoercesStringifiedLarkTaskPatchArgs(t *testing.T) {
+	bt := &BridgeTool{
+		inputSchema: map[string]any{
+			"type": "object",
+			"properties": map[string]any{
+				"path": map[string]any{
+					"type": "object",
+					"properties": map[string]any{
+						"task_guid": map[string]any{"type": "string"},
+					},
+				},
+				"params": map[string]any{
+					"type": "object",
+					"properties": map[string]any{
+						"user_id_type": map[string]any{"type": "string"},
+					},
+				},
+				"data": map[string]any{
+					"type": "object",
+					"properties": map[string]any{
+						"task": map[string]any{
+							"type": "object",
+							"properties": map[string]any{
+								"completed_at": map[string]any{"type": "string"},
+							},
+						},
+						"update_fields": map[string]any{
+							"type":  "array",
+							"items": map[string]any{"type": "string"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	args := map[string]any{
+		"path":   `{"task_guid":"2d446187-084d-4d45-883b-5d238242b9fc"}`,
+		"params": `{"user_id_type":"open_id"}`,
+		"data":   `{"task":{"completed_at":"1782906226000"},"update_fields":["completed_at"]}`,
+	}
+
+	normalized, coercedPaths, err := bt.normalizeArgsForSchema(args)
+	if err != nil {
+		t.Fatalf("normalizeArgsForSchema returned error: %v", err)
+	}
+
+	pathArg, ok := normalized["path"].(map[string]any)
+	if !ok {
+		t.Fatalf("path should be object after normalization, got %T", normalized["path"])
+	}
+	if pathArg["task_guid"] != "2d446187-084d-4d45-883b-5d238242b9fc" {
+		t.Errorf("unexpected task_guid: %v", pathArg["task_guid"])
+	}
+
+	dataArg, ok := normalized["data"].(map[string]any)
+	if !ok {
+		t.Fatalf("data should be object after normalization, got %T", normalized["data"])
+	}
+	taskArg, ok := dataArg["task"].(map[string]any)
+	if !ok {
+		t.Fatalf("data.task should be object after normalization, got %T", dataArg["task"])
+	}
+	if taskArg["completed_at"] != "1782906226000" {
+		t.Errorf("unexpected completed_at: %v", taskArg["completed_at"])
+	}
+
+	wantPaths := map[string]bool{"$.path": true, "$.params": true, "$.data": true}
+	for _, path := range coercedPaths {
+		delete(wantPaths, path)
+	}
+	if len(wantPaths) != 0 {
+		t.Errorf("missing coerced paths: %v; got %v", wantPaths, coercedPaths)
+	}
+}
+
+func TestNormalizeArgsForSchema_CoercesNestedStringifiedContainers(t *testing.T) {
+	bt := &BridgeTool{
+		inputSchema: map[string]any{
+			"type": "object",
+			"properties": map[string]any{
+				"data": map[string]any{
+					"type": "object",
+					"properties": map[string]any{
+						"task": map[string]any{
+							"type": "object",
+							"properties": map[string]any{
+								"completed_at": map[string]any{"type": "string"},
+							},
+						},
+						"update_fields": map[string]any{
+							"type":  "array",
+							"items": map[string]any{"type": "string"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	args := map[string]any{
+		"data": map[string]any{
+			"task":          `{"completed_at":"1782906226000"}`,
+			"update_fields": `["completed_at"]`,
+		},
+	}
+
+	normalized, coercedPaths, err := bt.normalizeArgsForSchema(args)
+	if err != nil {
+		t.Fatalf("normalizeArgsForSchema returned error: %v", err)
+	}
+
+	dataArg := normalized["data"].(map[string]any)
+	if _, ok := dataArg["task"].(map[string]any); !ok {
+		t.Fatalf("data.task should be object after normalization, got %T", dataArg["task"])
+	}
+	if fields, ok := dataArg["update_fields"].([]any); !ok || len(fields) != 1 || fields[0] != "completed_at" {
+		t.Fatalf("update_fields should be array after normalization, got %#v", dataArg["update_fields"])
+	}
+
+	wantPaths := map[string]bool{"$.data.task": true, "$.data.update_fields": true}
+	for _, path := range coercedPaths {
+		delete(wantPaths, path)
+	}
+	if len(wantPaths) != 0 {
+		t.Errorf("missing coerced paths: %v; got %v", wantPaths, coercedPaths)
+	}
+}
+
+func TestNormalizeArgsForSchema_RejectsStringifiedContainerWithWrongShape(t *testing.T) {
+	bt := &BridgeTool{
+		inputSchema: map[string]any{
+			"type": "object",
+			"properties": map[string]any{
+				"path": map[string]any{"type": "object"},
+			},
+		},
+	}
+
+	_, _, err := bt.normalizeArgsForSchema(map[string]any{"path": `["not","object"]`})
+	if err == nil {
+		t.Fatal("expected error for array string when schema requires object")
+	}
+	if !strings.Contains(err.Error(), "$.path must be object") {
+		t.Fatalf("expected actionable path/type error, got: %v", err)
+	}
+}
+
+func TestNormalizeArgsForSchema_DoesNotCoerceStringTypedProperty(t *testing.T) {
+	bt := &BridgeTool{
+		inputSchema: map[string]any{
+			"type": "object",
+			"properties": map[string]any{
+				"content": map[string]any{"type": "string"},
+			},
+		},
+	}
+
+	args := map[string]any{"content": `{"literal":"json text"}`}
+	normalized, coercedPaths, err := bt.normalizeArgsForSchema(args)
+	if err != nil {
+		t.Fatalf("normalizeArgsForSchema returned error: %v", err)
+	}
+	if normalized["content"] != `{"literal":"json text"}` {
+		t.Errorf("string-typed property should remain unchanged, got %#v", normalized["content"])
+	}
+	if len(coercedPaths) != 0 {
+		t.Errorf("string-typed property should not be coerced, got paths %v", coercedPaths)
+	}
+}
+
 func TestEnsureMCPPrefix(t *testing.T) {
 	tests := []struct {
 		name       string