diff --git a/pkg/github/__toolsnaps__/set_issue_fields.snap b/pkg/github/__toolsnaps__/set_issue_fields.snap index 7546ddc37..979dde4fb 100644 --- a/pkg/github/__toolsnaps__/set_issue_fields.snap +++ b/pkg/github/__toolsnaps__/set_issue_fields.snap @@ -27,6 +27,11 @@ "description": "The value to set for a number field", "type": "number" }, + "rationale": { + "description": "One concise sentence explaining what specifically about the issue led you to choose this field value. State the concrete signal (e.g. 'Reports a crash when saving' → high priority).", + "maxLength": 280, + "type": "string" + }, "single_select_option_id": { "description": "The GraphQL node ID of the option to set for a single select field", "type": "string" diff --git a/pkg/github/__toolsnaps__/update_issue_labels.snap b/pkg/github/__toolsnaps__/update_issue_labels.snap index 3acf98d93..89ff86b2f 100644 --- a/pkg/github/__toolsnaps__/update_issue_labels.snap +++ b/pkg/github/__toolsnaps__/update_issue_labels.snap @@ -13,9 +13,31 @@ "type": "number" }, "labels": { - "description": "Labels to apply to this issue", + "description": "Labels to apply to this issue.", "items": { - "type": "string" + "oneOf": [ + { + "description": "Label name", + "type": "string" + }, + { + "properties": { + "name": { + "description": "Label name", + "type": "string" + }, + "rationale": { + "description": "One concise sentence explaining what specifically about the issue led you to choose this label. State the concrete signal (e.g. 'Reports a crash when saving' → bug).", + "maxLength": 280, + "type": "string" + } + }, + "required": [ + "name" + ], + "type": "object" + } + ] }, "type": "array" }, diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 72ed1939d..59eb47822 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -263,24 +263,124 @@ func TestGranularUpdateIssueAssignees(t *testing.T) { } func TestGranularUpdateIssueLabels(t *testing.T) { - client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ - "labels": []any{"bug", "enhancement"}, - }).andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), - })) - deps := BaseDeps{Client: client} - serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) + tests := []struct { + name string + requestArgs map[string]any + expectedReq map[string]any + }{ + { + name: "labels as plain strings", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []any{"bug", "enhancement"}, + }, + expectedReq: map[string]any{ + "labels": []any{"bug", "enhancement"}, + }, + }, + { + name: "label objects without rationale serialize as strings", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []any{ + map[string]any{"name": "bug"}, + "enhancement", + }, + }, + expectedReq: map[string]any{ + "labels": []any{"bug", "enhancement"}, + }, + }, + { + name: "mixed strings and label objects with rationale", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []any{ + "triage", + map[string]any{"name": "bug", "rationale": " Reports a crash when saving "}, + map[string]any{"name": "frontend", "rationale": "Mentions the UI button"}, + }, + }, + expectedReq: map[string]any{ + "labels": []any{ + "triage", + map[string]any{"name": "bug", "rationale": "Reports a crash when saving"}, + map[string]any{"name": "frontend", "rationale": "Mentions the UI button"}, + }, + }, + }, + } - request := createMCPRequest(map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "labels": []string{"bug", "enhancement"}, - }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.False(t, result.IsError) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq). + andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), + })) + deps := BaseDeps{Client: client} + serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + } +} + +func TestGranularUpdateIssueLabelsInvalidRationale(t *testing.T) { + tests := []struct { + name string + requestArgs map[string]any + expectedErrText string + }{ + { + name: "rationale too long", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []any{ + map[string]any{"name": "bug", "rationale": strings.Repeat("a", 281)}, + }, + }, + expectedErrText: "label rationale must be 280 characters or less", + }, + { + name: "label object missing name", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "labels": []any{ + map[string]any{"rationale": "no name provided"}, + }, + }, + expectedErrText: "each label object must have a 'name' string", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))} + serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrText) + }) + } } func TestGranularUpdateIssueMilestone(t *testing.T) { @@ -1034,4 +1134,117 @@ func TestGranularSetIssueFields(t *testing.T) { textContent := getTextResult(t, result) assert.Contains(t, textContent.Text, "each field must have exactly one value") }) + + t.Run("successful set with text value and rationale", func(t *testing.T) { + matchers := []githubv4mock.Matcher{ + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(5), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{"id": "ISSUE_123"}, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + SetIssueFieldValue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + } + IssueFieldValues []struct { + TextValue struct { + Value string + } `graphql:"... on IssueFieldTextValue"` + SingleSelectValue struct { + Name string + } `graphql:"... on IssueFieldSingleSelectValue"` + DateValue struct { + Value string + } `graphql:"... on IssueFieldDateValue"` + NumberValue struct { + Value float64 + } `graphql:"... on IssueFieldNumberValue"` + } + } `graphql:"setIssueFieldValue(input: $input)"` + }{}, + SetIssueFieldValueInput{ + IssueID: githubv4.ID("ISSUE_123"), + IssueFields: []IssueFieldCreateOrUpdateInput{ + { + FieldID: githubv4.ID("FIELD_1"), + TextValue: githubv4.NewString(githubv4.String("hello")), + Rationale: githubv4.NewString(githubv4.String("Reflects the reported severity")), + }, + }, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "setIssueFieldValue": map[string]any{ + "issue": map[string]any{ + "id": "ISSUE_123", + "number": 5, + "url": "https://github.com/owner/repo/issues/5", + }, + }, + }), + ), + } + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...)) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularSetIssueFields(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(5), + "fields": []any{ + map[string]any{ + "field_id": "FIELD_1", + "text_value": "hello", + "rationale": " Reflects the reported severity ", + }, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + + t.Run("rationale too long returns error", func(t *testing.T) { + deps := BaseDeps{} + serverTool := GranularSetIssueFields(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(5), + "fields": []any{ + map[string]any{ + "field_id": "FIELD_1", + "text_value": "hello", + "rationale": strings.Repeat("a", 281), + }, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "field rationale must be 280 characters or less") + }) } diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index 5b335bd44..400a22f5c 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -258,31 +258,182 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor ) } +// labelWithRationale represents the object form of a label entry, allowing a +// rationale to be sent alongside the label name. +type labelWithRationale struct { + Name string `json:"name"` + Rationale string `json:"rationale,omitempty"` +} + +// labelsUpdateRequest is a custom request body for updating an issue's labels +// where individual labels may optionally include a rationale. Each element of +// Labels is either a string (label name) or a labelWithRationale object. +type labelsUpdateRequest struct { + Labels []any `json:"labels"` +} + // GranularUpdateIssueLabels creates a tool to update an issue's labels. func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.ServerTool { - return issueUpdateTool(t, - "update_issue_labels", - "Update the labels of an existing issue. This replaces the current labels with the provided list.", - "Update Issue Labels", - map[string]*jsonschema.Schema{ - "labels": { - Type: "array", - Description: "Labels to apply to this issue", - Items: &jsonschema.Schema{Type: "string"}, + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "update_issue_labels", + Description: t("TOOL_UPDATE_ISSUE_LABELS_DESCRIPTION", "Update the labels of an existing issue. This replaces the current labels with the provided list."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UPDATE_ISSUE_LABELS_USER_TITLE", "Update Issue Labels"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + OpenWorldHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner (username or organization)", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "The issue number to update", + Minimum: jsonschema.Ptr(1.0), + }, + "labels": { + Type: "array", + Description: "Labels to apply to this issue.", + Items: &jsonschema.Schema{ + OneOf: []*jsonschema.Schema{ + {Type: "string", Description: "Label name"}, + { + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "Label name", + }, + "rationale": { + Type: "string", + Description: "One concise sentence explaining what specifically about the issue led you to choose this label. " + + "State the concrete signal (e.g. 'Reports a crash when saving' → bug).", + MaxLength: jsonschema.Ptr(280), + }, + }, + Required: []string{"name"}, + }, + }, + }, + }, + }, + Required: []string{"owner", "repo", "issue_number", "labels"}, }, }, - []string{"labels"}, - func(args map[string]any) (*github.IssueRequest, error) { - if _, ok := args["labels"]; !ok { - return nil, fmt.Errorf("missing required parameter: labels") + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } - labels, err := OptionalStringArrayParam(args, "labels") + repo, err := RequiredParam[string](args, "repo") if err != nil { - return nil, err + return utils.NewToolResultError(err.Error()), nil, nil + } + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + labelsRaw, ok := args["labels"] + if !ok { + return utils.NewToolResultError("missing required parameter: labels"), nil, nil + } + labelsSlice, ok := labelsRaw.([]any) + if !ok { + // Also accept []string for callers that pre-typed the array. + if strs, ok := labelsRaw.([]string); ok { + labelsSlice = make([]any, len(strs)) + for i, s := range strs { + labelsSlice[i] = s + } + } else { + return utils.NewToolResultError("parameter labels must be an array"), nil, nil + } + } + + anyRationale := false + payload := make([]any, 0, len(labelsSlice)) + for _, item := range labelsSlice { + switch v := item.(type) { + case string: + payload = append(payload, v) + case map[string]any: + name, err := RequiredParam[string](v, "name") + if err != nil { + return utils.NewToolResultError("each label object must have a 'name' string"), nil, nil + } + rationale, err := OptionalParam[string](v, "rationale") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + rationale = strings.TrimSpace(rationale) + if len([]rune(rationale)) > 280 { + return utils.NewToolResultError("label rationale must be 280 characters or less"), nil, nil + } + if rationale == "" { + payload = append(payload, name) + } else { + anyRationale = true + payload = append(payload, labelWithRationale{Name: name, Rationale: rationale}) + } + default: + return utils.NewToolResultError("each label must be a string or an object with 'name' and optional 'rationale'"), nil, nil + } } - return &github.IssueRequest{Labels: &labels}, nil + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + var body any + if anyRationale { + body = &labelsUpdateRequest{Labels: payload} + } else { + // Preserve the standard wire format when no rationale is supplied. + names := make([]string, len(payload)) + for i, p := range payload { + names[i] = p.(string) + } + body = &github.IssueRequest{Labels: &names} + } + + apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) + req, err := client.NewRequest(ctx, "PATCH", apiURL, body) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to create request", err), nil, nil + } + + issue := &github.Issue{} + resp, err := client.Do(req, issue) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + r, err := json.Marshal(MinimalResponse{ + ID: fmt.Sprintf("%d", issue.GetID()), + URL: issue.GetHTMLURL(), + }) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + return utils.NewToolResultText(string(r)), nil, nil }, ) + st.FeatureFlagEnable = FeatureFlagIssuesGranular + return st } // GranularUpdateIssueMilestone creates a tool to update an issue's milestone. @@ -714,6 +865,7 @@ type IssueFieldCreateOrUpdateInput struct { DateValue *githubv4.String `json:"dateValue,omitempty"` SingleSelectOptionID *githubv4.ID `json:"singleSelectOptionId,omitempty"` Delete *githubv4.Boolean `json:"delete,omitempty"` + Rationale *githubv4.String `json:"rationale,omitempty"` } // GranularSetIssueFields creates a tool to set issue field values on an issue using GraphQL. @@ -776,6 +928,12 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv Type: "boolean", Description: "Set to true to delete this field value", }, + "rationale": { + Type: "string", + Description: "One concise sentence explaining what specifically about the issue led you to choose this field value. " + + "State the concrete signal (e.g. 'Reports a crash when saving' → high priority).", + MaxLength: jsonschema.Ptr(280), + }, }, Required: []string{"field_id"}, }, @@ -874,6 +1032,20 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv return utils.NewToolResultError("each field must have exactly one value (text_value, number_value, date_value, single_select_option_id) or delete: true, but multiple were provided"), nil, nil } + if _, exists := fieldMap["rationale"]; exists { + rationale, err := OptionalParam[string](fieldMap, "rationale") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + rationale = strings.TrimSpace(rationale) + if len([]rune(rationale)) > 280 { + return utils.NewToolResultError("field rationale must be 280 characters or less"), nil, nil + } + if rationale != "" { + input.Rationale = githubv4.NewString(githubv4.String(rationale)) + } + } + issueFields = append(issueFields, input) }