diff --git a/README.md b/README.md index bd0963abf..a0eee2931 100644 --- a/README.md +++ b/README.md @@ -708,7 +708,7 @@ The following sets of tools are available (all are on by default): - `query`: Filter projects by a search query (matches title and description) (string, optional) - **update_project_item** - Update project item - - `fields`: A list of field updates to apply. (array, required) + - `fields`: A list of field updates to apply. (object[], required) - `item_id`: The numeric ID of the project item to update (not the issue or pull request ID). (number, required) - `owner`: If owner_type == user it is the handle for the GitHub user account. If owner_type == org it is the name of the organization. The name is not case sensitive. (string, required) - `owner_type`: Owner type (string, required) diff --git a/pkg/github/__toolsnaps__/update_project_item.snap b/pkg/github/__toolsnaps__/update_project_item.snap index ff2905282..697691f30 100644 --- a/pkg/github/__toolsnaps__/update_project_item.snap +++ b/pkg/github/__toolsnaps__/update_project_item.snap @@ -8,6 +8,24 @@ "properties": { "fields": { "description": "A list of field updates to apply.", + "items": { + "additionalProperties": false, + "properties": { + "id": { + "description": "ID of the project field", + "type": "int" + }, + "value": { + "description": "value of the project field", + "type": "any" + } + }, + "required": [ + "id", + "value" + ], + "type": "object" + }, "type": "array" }, "item_id": { diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 09bcbd5ed..31d1fad7e 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -624,12 +624,44 @@ func DeleteProjectItem(getClient GetClientFn, t translations.TranslationHelperFu func UpdateProjectItem(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("update_project_item", mcp.WithDescription(t("TOOL_UPDATE_PROJECT_ITEM_DESCRIPTION", "Update a specific Project item for a user or org")), - mcp.WithToolAnnotation(mcp.ToolAnnotation{Title: t("TOOL_UPDATE_PROJECT_ITEM_USER_TITLE", "Update project item"), ReadOnlyHint: ToBoolPtr(false)}), - mcp.WithString("owner_type", mcp.Required(), mcp.Description("Owner type"), mcp.Enum("user", "org")), - mcp.WithString("owner", mcp.Required(), mcp.Description("If owner_type == user it is the handle for the GitHub user account. If owner_type == org it is the name of the organization. The name is not case sensitive.")), - mcp.WithNumber("project_number", mcp.Required(), mcp.Description("The project's number.")), - mcp.WithNumber("item_id", mcp.Required(), mcp.Description("The numeric ID of the project item to update (not the issue or pull request ID).")), - mcp.WithArray("fields", mcp.Required(), mcp.Description("A list of field updates to apply.")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{Title: t("TOOL_UPDATE_PROJECT_ITEM_USER_TITLE", "Update project item"), + ReadOnlyHint: ToBoolPtr(false)}, + ), + mcp.WithString("owner_type", + mcp.Required(), mcp.Description("Owner type"), mcp.Enum("user", "org"), + ), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("If owner_type == user it is the handle for the GitHub user account. If owner_type == org it is the name of the organization. The name is not case sensitive."), + ), + mcp.WithNumber("project_number", + mcp.Required(), mcp.Description("The project's number."), + ), + mcp.WithNumber("item_id", + mcp.Required(), + mcp.Description("The numeric ID of the project item to update (not the issue or pull request ID)."), + ), + mcp.WithArray("fields", + mcp.Required(), + mcp.Description("A list of field updates to apply."), + mcp.Items( + map[string]interface{}{ + "type": "object", + "additionalProperties": false, + "required": []string{"id", "value"}, + "properties": map[string]interface{}{ + "id": map[string]interface{}{ + "type": "int", + "description": "ID of the project field", + }, + "value": map[string]interface{}{ + "type": "any", + // intentionally left without a specific JSON schema type to allow any value + "description": "value of the project field", + }, + }, + }), + ), ), func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := RequiredParam[string](req, "owner") if err != nil { @@ -651,16 +683,18 @@ func UpdateProjectItem(getClient GetClientFn, t translations.TranslationHelperFu if err != nil { return mcp.NewToolResultError(err.Error()), nil } - fieldsParam, ok := req.GetArguments()["fields"] - if !ok { + + // Extract and validate fields parameter (required & non-empty) + args := req.GetArguments() + rawFields, present := args["fields"] + if !present { return mcp.NewToolResultError("missing required parameter: fields"), nil } - - rawFields, ok := fieldsParam.([]any) + fields, ok := rawFields.([]interface{}) if !ok { - return mcp.NewToolResultError("parameter fields must be an array of objects"), nil + return mcp.NewToolResultError("fields parameter must be an array of objects with id and value"), nil } - if len(rawFields) == 0 { + if len(fields) == 0 { return mcp.NewToolResultError("fields must contain at least one field update"), nil } @@ -671,43 +705,32 @@ func UpdateProjectItem(getClient GetClientFn, t translations.TranslationHelperFu projectsURL = fmt.Sprintf("users/%s/projectsV2/%d/items/%d", owner, projectNumber, itemID) } - updateFields := make([]*newProjectV2Field, 0, len(rawFields)) - for idx, rawField := range rawFields { - fieldMap, ok := rawField.(map[string]any) + updateFields := make([]*newProjectV2Field, 0, len(fields)) + for _, rawField := range fields { + m, ok := rawField.(map[string]any) if !ok { - return mcp.NewToolResultError(fmt.Sprintf("fields[%d] must be an object", idx)), nil + return mcp.NewToolResultError("each element of fields must be an object"), nil } - - rawID, ok := fieldMap["id"] + idVal, ok := m["id"] if !ok { - return mcp.NewToolResultError(fmt.Sprintf("fields[%d] is missing 'id'", idx)), nil + return mcp.NewToolResultError("each field update must include an 'id'"), nil } - - var fieldID int64 - switch v := rawID.(type) { + var idInt64 int64 + switch v := idVal.(type) { case float64: - fieldID = int64(v) + idInt64 = int64(v) + case int: + idInt64 = int64(v) case int64: - fieldID = v - case json.Number: - n, convErr := v.Int64() - if convErr != nil { - return mcp.NewToolResultError(fmt.Sprintf("fields[%d].id must be a numeric value", idx)), nil - } - fieldID = n + idInt64 = v default: - return mcp.NewToolResultError(fmt.Sprintf("fields[%d].id must be a numeric value", idx)), nil + return mcp.NewToolResultError("field 'id' must be a number"), nil } - - value, ok := fieldMap["value"] + value, ok := m["value"] if !ok { - return mcp.NewToolResultError(fmt.Sprintf("fields[%d] is missing 'value'", idx)), nil + return mcp.NewToolResultError("each field update must include a 'value'"), nil } - - updateFields = append(updateFields, &newProjectV2Field{ - ID: github.Ptr(fieldID), - Value: value, - }) + updateFields = append(updateFields, &newProjectV2Field{ID: github.Ptr(idInt64), Value: value}) } updateProjectItemOptions := &updateProjectItemOptions{Fields: updateFields} @@ -749,8 +772,9 @@ type updateProjectItemOptions struct { } type newProjectV2Field struct { - ID *int64 `json:"id,omitempty"` - Value any `json:"value,omitempty"` + ID *int64 `json:"id,omitempty"` + // Value should be sent as explicit null if user supplies null, so do NOT use omitempty + Value any `json:"value"` } type newProjectItem struct { diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 628bad8fb..dd31ab722 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -1383,6 +1383,82 @@ func Test_UpdateProjectItem(t *testing.T) { expectedID: 801, expectedCreatorLogin: "octocat", }, + { + name: "success organization multi-field update", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{Pattern: "/orgs/{org}/projectsV2/{project}/items/{item_id}", Method: http.MethodPatch}, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + var payload struct { + Fields []struct { + ID int `json:"id"` + Value interface{} `json:"value"` + } `json:"fields"` + } + assert.NoError(t, json.Unmarshal(body, &payload)) + assert.Len(t, payload.Fields, 2) + if len(payload.Fields) == 2 { + assert.Equal(t, 1111, payload.Fields[0].ID) + assert.Equal(t, "Backlog", payload.Fields[0].Value) + assert.Equal(t, 2222, payload.Fields[1].ID) + assert.Equal(t, 5.0, payload.Fields[1].Value) // JSON numbers -> float64 + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write(mock.MustMarshal(orgUpdated)) + }), + ), + ), + requestArgs: map[string]any{ + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(9999), + "item_id": float64(8888), + "fields": []any{ + map[string]any{"id": float64(1111), "value": "Backlog"}, + map[string]any{"id": float64(2222), "value": float64(5)}, + }, + }, + expectedID: 801, + expectedCreatorLogin: "octocat", + }, + { + name: "success organization update null value", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{Pattern: "/orgs/{org}/projectsV2/{project}/items/{item_id}", Method: http.MethodPatch}, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + var raw map[string]any + assert.NoError(t, json.Unmarshal(body, &raw)) + fieldsRaw, ok := raw["fields"].([]any) + assert.True(t, ok) + assert.Len(t, fieldsRaw, 1) + first, ok := fieldsRaw[0].(map[string]any) + assert.True(t, ok) + // Ensure key exists and is explicitly null (presence matters) + val, present := first["value"] + assert.True(t, present) + assert.Nil(t, val) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(mock.MustMarshal(orgUpdated)) + }), + ), + ), + requestArgs: map[string]any{ + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1010), + "item_id": float64(2020), + "fields": []any{ + map[string]any{"id": float64(999), "value": nil}, + }, + }, + expectedID: 801, + expectedCreatorLogin: "octocat", + }, { name: "success user update", mockedClient: mock.NewMockedHTTPClient( @@ -1502,6 +1578,36 @@ func Test_UpdateProjectItem(t *testing.T) { expectError: true, expectedErrMsg: "fields must contain at least one field update", }, + { + name: "field missing id", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "item_id": float64(1), + "fields": []any{ + map[string]any{"value": "X"}, + }, + }, + expectError: true, + expectedErrMsg: "each field update must include an 'id'", + }, + { + name: "field missing value", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "item_id": float64(1), + "fields": []any{ + map[string]any{"id": float64(55)}, + }, + }, + expectError: true, + expectedErrMsg: "each field update must include a 'value'", + }, } for _, tc := range tests {