diff --git a/internal/mcp/bridge_tool.go b/internal/mcp/bridge_tool.go index 43bcbb35..51c4de78 100644 --- a/internal/mcp/bridge_tool.go +++ b/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 diff --git a/internal/mcp/bridge_tool_arg_normalization.go b/internal/mcp/bridge_tool_arg_normalization.go new file mode 100644 index 00000000..579e4a56 --- /dev/null +++ b/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 +} diff --git a/internal/mcp/bridge_tool_schema_helpers.go b/internal/mcp/bridge_tool_schema_helpers.go new file mode 100644 index 00000000..b31cab7e --- /dev/null +++ b/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) + } +} diff --git a/internal/mcp/bridge_tool_test.go b/internal/mcp/bridge_tool_test.go index 787862dc..55de8302 100644 --- a/internal/mcp/bridge_tool_test.go +++ b/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