From c330ae66b45228e8ee0ed87969994cfc3b607096 Mon Sep 17 00:00:00 2001 From: Rafael Dantas Justo Date: Fri, 24 Jul 2026 09:46:09 -0300 Subject: [PATCH] Fix: Coerce numeric dropdown/status/multiselect custom field values to strings Setting a choice-based custom field value via create/update_custom_field_value failed for numeric input: the value was forwarded verbatim, so a JSON number reached the API where dropdown/status/multiselect choices are stored as strings, yielding "cannot unmarshal number ... into string" (dropdown custom fields could not be set through the connector at all). Resolve the field type and stringify the value for dropdown, status and multiselect fields before sending; number, checkbox, date and other types pass through unchanged. The field lookup only runs when the value actually carries a number, so the common string-value path adds no extra call. Also correct the misleading "option IDs for dropdown" wording in the value schema and allow numeric elements in the multiselect array branch. Adds a routed mock that captures the request body so tests assert the coerced wire payload. Co-Authored-By: Claude Opus 4.8 (1M context) --- go.mod | 2 +- go.sum | 4 +- internal/testutil/mcp.go | 36 ++++++ internal/twprojects/custom_field_values.go | 120 ++++++++++++++++-- .../twprojects/custom_field_values_test.go | 90 +++++++++++++ 5 files changed, 237 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index 6ff6803..31dbc86 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/modelcontextprotocol/go-sdk v1.6.1 github.com/teamwork/desksdkgo v1.0.1 github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb - github.com/teamwork/twapi-go-sdk v1.20.2 + github.com/teamwork/twapi-go-sdk v1.20.3 ) require ( diff --git a/go.sum b/go.sum index 89455d6..75f14ae 100644 --- a/go.sum +++ b/go.sum @@ -184,8 +184,8 @@ github.com/teamwork/desksdkgo v1.0.1 h1:Mi5D1pnGfL5JwD7s7g7OyHml7Y9jsak5MNJaotJt github.com/teamwork/desksdkgo v1.0.1/go.mod h1:Mgvw83q8iqHr7Sm9xV1iI/T89o3ObaPU3ChMJheRzwA= github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb h1:bQluDjySZeC5etnWgjk4WFRy0PvzGDw8XEBd4JJYWCQ= github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb/go.mod h1:jfE0RLsZuk/3Glzs5bJ95pNb92emV7uXZYgoGSLQ76I= -github.com/teamwork/twapi-go-sdk v1.20.2 h1:wO9c28OXtwDXjeAZB8u44OkotKsAaaBVgO8B+9hYfew= -github.com/teamwork/twapi-go-sdk v1.20.2/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= +github.com/teamwork/twapi-go-sdk v1.20.3 h1:W4V2adgnrdCqdAupUgm1ElurmFpaND8fOUI/SuH2gBU= +github.com/teamwork/twapi-go-sdk v1.20.3/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/tinylib/msgp v1.6.3 h1:bCSxiTz386UTgyT1i0MSCvdbWjVW+8sG3PjkGsZQt4s= github.com/tinylib/msgp v1.6.3/go.mod h1:RSp0LW9oSxFut3KzESt5Voq4GVWyS+PSulT77roAqEA= github.com/tklauser/go-sysconf v0.3.16 h1:frioLaCQSsF5Cy1jgRBrzr6t502KIIwQ0MArYICU0nA= diff --git a/internal/testutil/mcp.go b/internal/testutil/mcp.go index d94d009..423bb33 100644 --- a/internal/testutil/mcp.go +++ b/internal/testutil/mcp.go @@ -183,6 +183,42 @@ func ProjectsMCPServerRoutedMock( return mcpServer } +// ProjectsMCPServerRoutedMockWithRequestBody is like ProjectsMCPServerRoutedMock +// but also captures the body of the most recent HTTP request that carried one, +// so tests can assert on the serialized payload of the final write while still +// serving distinct responses per endpoint (e.g. a field-type GET at 200 +// followed by a value POST at 201). +func ProjectsMCPServerRoutedMockWithRequestBody( + t *testing.T, + routes []ProjectsMockRoute, + fallbackStatus int, + fallbackBody []byte, +) (*mcp.Server, *[]byte) { + t.Helper() + + var lastBody []byte + engine := twapi.NewEngine(ProjectsSessionMock{}, twapi.WithMiddleware(func(twapi.HTTPClient) twapi.HTTPClient { + return twapi.HTTPClientFunc(func(req *http.Request) (*http.Response, error) { + if req.Body != nil { + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + lastBody = body + } + path := req.URL.Path + for _, route := range routes { + if strings.Contains(path, route.Match) { + return newProjectsMockHTTPResponse(route.Status, route.Body), nil + } + } + return newProjectsMockHTTPResponse(fallbackStatus, fallbackBody), nil + }) + })) + + return projectsMCPServer(t, engine), &lastBody +} + // ProjectsMCPServerSequencedMock creates a mock MCP server for twprojects // testing whose engine returns the given response bodies in order, one per HTTP // request the engine makes. Once the sequence is exhausted the final body is diff --git a/internal/twprojects/custom_field_values.go b/internal/twprojects/custom_field_values.go index 0fb3368..b32681b 100644 --- a/internal/twprojects/custom_field_values.go +++ b/internal/twprojects/custom_field_values.go @@ -5,7 +5,9 @@ import ( "encoding/json" "fmt" "io" + "math" "net/http" + "strconv" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -79,11 +81,9 @@ func CustomFieldValueCreate(engine *twapi.Engine) toolsets.ToolWrapper { Description: "The ID of the custom field the value belongs to.", }, "value": { - Description: "The value to assign to the custom field. " + - "The concrete type depends on the custom field definition: " + - "strings for text fields, numbers for number fields, booleans for checkboxes, " + - "option IDs for dropdown fields (array of integers for multiselect), " + - "ISO-8601 date strings for date fields.", + Description: "The value to assign, typed per the field: string (text), " + + "number (number), boolean (checkbox), choice value string " + + "(dropdown/status; array for multiselect), ISO-8601 string (date).", AnyOf: []*jsonschema.Schema{ {Type: "string"}, {Type: "number"}, @@ -91,7 +91,10 @@ func CustomFieldValueCreate(engine *twapi.Engine) toolsets.ToolWrapper { { Type: "array", Items: &jsonschema.Schema{ - Type: "string", + AnyOf: []*jsonschema.Schema{ + {Type: "string"}, + {Type: "number"}, + }, }, }, {Type: "null"}, @@ -145,6 +148,7 @@ func CustomFieldValueCreate(engine *twapi.Engine) toolsets.ToolWrapper { if !ok { return helpers.NewToolResultTextError("invalid parameters: 'value' is required"), nil } + value = coerceCustomFieldValue(ctx, engine, customFieldID, value) var customFieldValueCreateRequest projects.CustomFieldValueCreateRequest switch entity { @@ -203,11 +207,9 @@ func CustomFieldValueUpdate(engine *twapi.Engine) toolsets.ToolWrapper { Description: "The ID of the custom field the value belongs to.", }, "value": { - Description: "The value to assign to the custom field. " + - "The concrete type depends on the custom field definition: " + - "strings for text fields, numbers for number fields, booleans for checkboxes, " + - "option IDs for dropdown fields (array of integers for multiselect), " + - "ISO-8601 date strings for date fields.", + Description: "The value to assign, typed per the field: string (text), " + + "number (number), boolean (checkbox), choice value string " + + "(dropdown/status; array for multiselect), ISO-8601 string (date).", AnyOf: []*jsonschema.Schema{ {Type: "string"}, {Type: "number"}, @@ -215,7 +217,10 @@ func CustomFieldValueUpdate(engine *twapi.Engine) toolsets.ToolWrapper { { Type: "array", Items: &jsonschema.Schema{ - Type: "string", + AnyOf: []*jsonschema.Schema{ + {Type: "string"}, + {Type: "number"}, + }, }, }, {Type: "null"}, @@ -267,6 +272,7 @@ func CustomFieldValueUpdate(engine *twapi.Engine) toolsets.ToolWrapper { } value := arguments["value"] + value = coerceCustomFieldValue(ctx, engine, customFieldID, value) var customFieldValueUpdateRequest projects.CustomFieldValueUpdateRequest switch entity { @@ -574,3 +580,93 @@ func CustomFieldValueList(engine *twapi.Engine) toolsets.ToolWrapper { }, } } + +// coerceCustomFieldValue normalizes the raw value into the wire shape the +// Teamwork API expects. Dropdown and multiselect custom fields store their +// choices as strings, but MCP clients frequently send a choice as a JSON +// number (the tool schema also permits numbers, for number-type fields). A +// numeric value posted to a dropdown field is rejected by the API with +// "cannot unmarshal number ... into string", so when the raw value carries a +// number we resolve the field type and stringify it for dropdown/multiselect +// fields. Every other case is passed through untouched; if the field type +// cannot be resolved we forward the value unchanged and let the API validate. +func coerceCustomFieldValue(ctx context.Context, engine *twapi.Engine, customFieldID int64, raw any) any { + if customFieldID == 0 || !containsNumber(raw) { + return raw + } + resp, err := projects.CustomFieldGet(ctx, engine, projects.NewCustomFieldGetRequest(customFieldID)) + if err != nil { + return raw + } + // Dropdown, status and multiselect fields store their choices as strings, + // so a numeric choice must be stringified. Dropdown and status are + // single-valued; multiselect is an array. + switch resp.CustomField.Type { + case projects.CustomFieldTypeDropdown, projects.CustomFieldTypeStatus: + if s, ok := stringifyScalar(raw); ok { + return s + } + case projects.CustomFieldTypeMultiselect: + if arr, ok := raw.([]any); ok { + out := make([]any, len(arr)) + for i, v := range arr { + if s, ok := stringifyScalar(v); ok { + out[i] = s + } else { + out[i] = v + } + } + return out + } + if s, ok := stringifyScalar(raw); ok { + return s + } + } + return raw +} + +// containsNumber reports whether raw is a JSON number, or an array holding at +// least one number. It is the cheap pre-check that lets coerceCustomFieldValue +// skip the field lookup for the common case of already-string values. +func containsNumber(raw any) bool { + switch v := raw.(type) { + case float64, float32, int, int32, int64, json.Number: + return true + case []any: + for _, e := range v { + if containsNumber(e) { + return true + } + } + } + return false +} + +// stringifyScalar renders a scalar JSON value as the string the API expects for +// dropdown choices. Integer-valued numbers are rendered without a trailing +// decimal (1 -> "1", not "1.000000"). It reports false for values that have no +// meaningful scalar string form (e.g. arrays or objects). +func stringifyScalar(raw any) (string, bool) { + switch v := raw.(type) { + case string: + return v, true + case float64: + if !math.IsInf(v, 0) && !math.IsNaN(v) && v == math.Trunc(v) { + return strconv.FormatInt(int64(v), 10), true + } + return strconv.FormatFloat(v, 'f', -1, 64), true + case float32: + return stringifyScalar(float64(v)) + case int: + return strconv.FormatInt(int64(v), 10), true + case int32: + return strconv.FormatInt(int64(v), 10), true + case int64: + return strconv.FormatInt(v, 10), true + case bool: + return strconv.FormatBool(v), true + case json.Number: + return v.String(), true + } + return "", false +} diff --git a/internal/twprojects/custom_field_values_test.go b/internal/twprojects/custom_field_values_test.go index 1d7381c..c380268 100644 --- a/internal/twprojects/custom_field_values_test.go +++ b/internal/twprojects/custom_field_values_test.go @@ -2,6 +2,7 @@ package twprojects_test import ( "net/http" + "strings" "testing" "github.com/teamwork/mcp/internal/testutil" @@ -38,6 +39,95 @@ func TestCustomFieldValueCreateMultiselect(t *testing.T) { }) } +func TestCustomFieldValueCreateDropdownCoercesNumberToString(t *testing.T) { + // A numeric value bound for a dropdown field must be stringified: the API + // stores dropdown choices as strings and rejects a raw number. The create + // handler resolves the field type via a GET (200) before the value POST + // (201), so route by URL to give each leg the right status. + mcpServer, body := testutil.ProjectsMCPServerRoutedMockWithRequestBody(t, []testutil.ProjectsMockRoute{ + {Match: "/customfields/555", Status: http.StatusOK, Body: []byte(`{"customfield":{"id":555,"type":"dropdown"}}`)}, + }, http.StatusCreated, []byte(`{"customfieldTask":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueCreate.String(), map[string]any{ + "entity": "task", + "entity_id": float64(777), + "custom_field_id": float64(555), + "value": float64(1), + }) + if got := string(*body); !strings.Contains(got, `"value":"1"`) { + t.Errorf("expected posted value to be stringified as \"1\", got body: %s", got) + } +} + +func TestCustomFieldValueCreateNumberFieldKeepsNumber(t *testing.T) { + // A numeric value bound for a number field must stay a number, not be + // stringified — only dropdown/multiselect choices are coerced. + mcpServer, body := testutil.ProjectsMCPServerRoutedMockWithRequestBody(t, []testutil.ProjectsMockRoute{ + {Match: "/customfields/555", Status: http.StatusOK, Body: []byte(`{"customfield":{"id":555,"type":"number-integer"}}`)}, + }, http.StatusCreated, []byte(`{"customfieldTask":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueCreate.String(), map[string]any{ + "entity": "task", + "entity_id": float64(777), + "custom_field_id": float64(555), + "value": float64(42), + }) + if got := string(*body); !strings.Contains(got, `"value":42`) { + t.Errorf("expected posted value to remain the number 42, got body: %s", got) + } +} + +func TestCustomFieldValueCreateStatusCoercesNumberToString(t *testing.T) { + // Status fields are choice-based strings too, so a numeric value must be + // stringified just like a dropdown. + mcpServer, body := testutil.ProjectsMCPServerRoutedMockWithRequestBody(t, []testutil.ProjectsMockRoute{ + {Match: "/customfields/555", Status: http.StatusOK, Body: []byte(`{"customfield":{"id":555,"type":"status"}}`)}, + }, http.StatusCreated, []byte(`{"customfieldTask":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueCreate.String(), map[string]any{ + "entity": "task", + "entity_id": float64(777), + "custom_field_id": float64(555), + "value": float64(5), + }) + if got := string(*body); !strings.Contains(got, `"value":"5"`) { + t.Errorf("expected posted value to be stringified as \"5\", got body: %s", got) + } +} + +func TestCustomFieldValueUpdateDropdownCoercesNumberToString(t *testing.T) { + // Same coercion applies on update. The field-type GET (custom_field_id=555) + // and the value PATCH (value_id=123) both live under /customfields/, so the + // route matches the field id specifically and the PATCH falls through. + mcpServer, body := testutil.ProjectsMCPServerRoutedMockWithRequestBody(t, []testutil.ProjectsMockRoute{ + {Match: "/customfields/555", Status: http.StatusOK, Body: []byte(`{"customfield":{"id":555,"type":"dropdown"}}`)}, + }, http.StatusOK, []byte(`{"customfieldTask":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueUpdate.String(), map[string]any{ + "entity": "task", + "entity_id": float64(777), + "value_id": float64(123), + "custom_field_id": float64(555), + "value": float64(3), + }) + if got := string(*body); !strings.Contains(got, `"value":"3"`) { + t.Errorf("expected patched value to be stringified as \"3\", got body: %s", got) + } +} + +func TestCustomFieldValueCreateMultiselectCoercesNumbersToStrings(t *testing.T) { + // Multiselect values arrive as an array; each numeric element must be + // stringified to match the stored choice values. + mcpServer, body := testutil.ProjectsMCPServerRoutedMockWithRequestBody(t, []testutil.ProjectsMockRoute{ + {Match: "/customfields/555", Status: http.StatusOK, Body: []byte(`{"customfield":{"id":555,"type":"multiselect"}}`)}, + }, http.StatusCreated, []byte(`{"customfieldTask":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueCreate.String(), map[string]any{ + "entity": "task", + "entity_id": float64(777), + "custom_field_id": float64(555), + "value": []any{float64(10), float64(20)}, + }) + if got := string(*body); !strings.Contains(got, `"value":["10","20"]`) { + t.Errorf("expected posted value to be stringified array, got body: %s", got) + } +} + func TestCustomFieldValueUpdate(t *testing.T) { mcpServer := mcpServerMock(t, http.StatusOK, []byte(`{"customfieldTask":{"id":123}}`)) testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCustomFieldValueUpdate.String(), map[string]any{