diff --git a/README.md b/README.md index 71147033a3..7c5b954c93 100644 --- a/README.md +++ b/README.md @@ -941,7 +941,7 @@ The following sets of tools are available: - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) - `title`: Issue title (string, optional) - - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) + - `type`: Type of this issue. For updates, use an empty string to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **list_issue_fields** - List issue fields - **Required OAuth Scopes (any of)**: `repo`, `read:org` diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 32891af62e..8b55a358ce 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -71,7 +71,7 @@ runtime behavior (such as output formatting) won't appear here. - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) - `title`: Issue title (string, optional) - - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) + - `type`: Type of this issue. For updates, use an empty string to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **ui_get** - Get UI data - **Required OAuth Scopes (any of)**: `repo`, `read:org` @@ -200,7 +200,7 @@ runtime behavior (such as output formatting) won't appear here. - `confidence`: How confident you are in this choice. Use 'HIGH' for clear signal or explicit user request, 'MEDIUM' for reasonable inference with some ambiguity, 'LOW' for best guess with limited signal. (string, optional) - `is_suggestion`: If true, this issue type change is sent to the API as a suggestion (suggest:true) rather than an applied value. Whether the type is applied or recorded as a proposal is determined by the API. (boolean, optional) - `issue_number`: The issue number to update (number, required) - - `issue_type`: The issue type to set (string, required) + - `issue_type`: The issue type to set, or an empty string to remove the current type (string, required) - `owner`: Repository owner (username or organization) (string, required) - `rationale`: One concise sentence explaining what specifically about the issue led you to choose this type. State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature). (string, optional) - `repo`: Repository name (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 10df187a91..11270dc662 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -65,7 +65,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) - `title`: Issue title (string, optional) - - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) + - `type`: Type of this issue. For updates, use an empty string to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (string, optional) - **ui_get** - Get UI data - **Required OAuth Scopes (any of)**: `repo`, `read:org` diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index 55fd2dbcc2..4f790eb9f1 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -120,7 +120,7 @@ "type": "string" }, "type": { - "description": "Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", + "description": "Type of this issue. For updates, use an empty string to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", "type": "string" } }, diff --git a/pkg/github/__toolsnaps__/update_issue_type.snap b/pkg/github/__toolsnaps__/update_issue_type.snap index 21a2f64bd5..98fbc16d8c 100644 --- a/pkg/github/__toolsnaps__/update_issue_type.snap +++ b/pkg/github/__toolsnaps__/update_issue_type.snap @@ -6,7 +6,7 @@ "readOnlyHint": false, "title": "Update Issue Type" }, - "description": "Update the type of an existing issue (e.g. 'bug', 'feature'). When setting values, include a confidence level (LOW, MEDIUM, or HIGH) reflecting how certain you are about the choice.", + "description": "Set or remove the type of an existing issue. Use an empty issue_type to remove the current type. When setting a value, include a confidence level (LOW, MEDIUM, or HIGH) reflecting how certain you are about the choice.", "inputSchema": { "properties": { "confidence": { @@ -28,7 +28,7 @@ "type": "number" }, "issue_type": { - "description": "The issue type to set", + "description": "The issue type to set, or an empty string to remove the current type", "type": "string" }, "owner": { diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 58fd904e88..85e29c7b2f 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -14,6 +14,7 @@ import ( "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" gogithub "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -754,6 +755,9 @@ func TestGranularUpdateIssueMilestone(t *testing.T) { } func TestGranularUpdateIssueType(t *testing.T) { + toolSchema := GranularUpdateIssueType(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + assert.Equal(t, "string", toolSchema.Properties["issue_type"].Type) + tests := []struct { name string requestArgs map[string]any @@ -787,6 +791,18 @@ func TestGranularUpdateIssueType(t *testing.T) { }, }, }, + { + name: "remove type with empty string", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "issue_type": "", + }, + expectedReq: map[string]any{ + "type": nil, + }, + }, } for _, tc := range tests { @@ -807,6 +823,23 @@ func TestGranularUpdateIssueType(t *testing.T) { } } +func TestGranularUpdateIssueTypeRejectsMissingIssueType(t *testing.T) { + deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))} + serverTool := GranularUpdateIssueType(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, "missing required parameter: issue_type") +} + func TestGranularUpdateIssueTypeSuggest(t *testing.T) { tests := []struct { name string diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 083c5465e0..f73b5fcf0a 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2151,7 +2151,7 @@ Options are: }, "type": { Type: "string", - Description: "Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", + Description: "Type of this issue. For updates, use an empty string to remove the current type. Only use if issue types are enabled for this repository. Use list_issue_types to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", }, "state": { Type: "string", @@ -2224,7 +2224,15 @@ Options are: // Hand off to the interactive MCP App form unless this call must // execute now (see shouldDeferToForm). - if shouldDeferToForm(ctx, deps, req, args, issueWriteFormParams) { + deferToForm := shouldDeferToForm(ctx, deps, req, args, issueWriteFormParams) + if method == "update" { + if issueType, ok := args["type"].(string); ok && issueType == "" { + // The form replaces an empty type with the current type, so execute + // directly to preserve the clear and any co-submitted values. + deferToForm = false + } + } + if deferToForm { issueNumber := 0 if method == "update" { n, numErr := RequiredInt(args, "issue_number") @@ -2275,7 +2283,7 @@ Options are: } // Get optional type - issueType, err := OptionalParam[string](args, "type") + issueType, issueTypeProvided, err := OptionalParamOK[string](args, "type") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -2336,6 +2344,7 @@ Options are: result, err := UpdateIssue(ctx, client, gqlClient, owner, repo, issueNumber, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues, fieldIDsToDelete, state, stateReason, duplicateOf, UpdateIssueOptions{ AssigneesProvided: assigneesProvided, LabelsProvided: labelsProvided, + IssueTypeProvided: issueTypeProvided, }) return result, nil, err default: @@ -2406,6 +2415,8 @@ type UpdateIssueOptions struct { AssigneesProvided bool // LabelsProvided sends the labels field even when the slice is empty. LabelsProvided bool + // IssueTypeProvided sends the type field, including an explicit clear for an empty value. + IssueTypeProvided bool } func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner string, repo string, issueNumber int, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string, issueFieldValues []*github.IssueRequestFieldValue, fieldIDsToDelete []int64, state string, stateReason string, duplicateOf int, opts ...UpdateIssueOptions) (*mcp.CallToolResult, error) { @@ -2416,6 +2427,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 for _, opt := range opts { updateOptions.AssigneesProvided = updateOptions.AssigneesProvided || opt.AssigneesProvided updateOptions.LabelsProvided = updateOptions.LabelsProvided || opt.LabelsProvided + updateOptions.IssueTypeProvided = updateOptions.IssueTypeProvided || opt.IssueTypeProvided } // Create the issue request with only provided fields @@ -2489,7 +2501,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 } } - updatedIssue, resp, err := client.Issues.Update(ctx, owner, repo, issueNumber, issueRequest) + updatedIssue, resp, err := patchIssue(ctx, client, owner, repo, issueNumber, issueRequest, issueType, updateOptions.IssueTypeProvided) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", @@ -2622,6 +2634,28 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return utils.NewToolResultText(string(r)), nil } +type updateIssueRequestWithNullableType struct { + github.UpdateIssueRequest + Type *string `json:"type"` +} + +func patchIssue(ctx context.Context, client *github.Client, owner, repo string, issueNumber int, issueRequest github.UpdateIssueRequest, issueType string, issueTypeProvided bool) (*github.Issue, *github.Response, error) { + if !issueTypeProvided || issueType != "" { + return client.Issues.Update(ctx, owner, repo, issueNumber, issueRequest) + } + + apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) + body := &updateIssueRequestWithNullableType{UpdateIssueRequest: issueRequest} + req, err := client.NewRequest(ctx, http.MethodPatch, apiURL, body) + if err != nil { + return nil, nil, err + } + + issue := &github.Issue{} + resp, err := client.Do(req, issue) + return issue, resp, err +} + // ListIssues creates a tool to list issues in a GitHub repository. func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index 314ead3eb4..64161acacd 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -679,13 +679,13 @@ type issueTypeUpdateRequest struct { Type issueTypeWithIntent `json:"type"` } -// GranularUpdateIssueType creates a tool to update an issue's type. +// GranularUpdateIssueType creates a tool to set or clear an issue's type. func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "update_issue_type", - Description: t("TOOL_UPDATE_ISSUE_TYPE_DESCRIPTION", "Update the type of an existing issue (e.g. 'bug', 'feature'). When setting values, include a confidence level (LOW, MEDIUM, or HIGH) reflecting how certain you are about the choice."), + Description: t("TOOL_UPDATE_ISSUE_TYPE_DESCRIPTION", "Set or remove the type of an existing issue. Use an empty issue_type to remove the current type. When setting a value, include a confidence level (LOW, MEDIUM, or HIGH) reflecting how certain you are about the choice."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_UPDATE_ISSUE_TYPE_USER_TITLE", "Update Issue Type"), ReadOnlyHint: false, @@ -710,7 +710,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser }, "issue_type": { Type: "string", - Description: "The issue type to set", + Description: "The issue type to set, or an empty string to remove the current type", }, "rationale": { Type: "string", @@ -746,10 +746,13 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - issueType, err := RequiredParam[string](args, "issue_type") + issueType, issueTypeProvided, err := OptionalParamOK[string](args, "issue_type") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if !issueTypeProvided { + return utils.NewToolResultError("missing required parameter: issue_type"), nil, nil + } rationale, err := OptionalParam[string](args, "rationale") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -770,14 +773,16 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - client, err := deps.GetClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } var body any - if rationale != "" || isSuggestion || confidence != "" { + switch { + case issueType == "": + body = map[string]any{"type": nil} + case rationale != "" || isSuggestion || confidence != "": body = &issueTypeUpdateRequest{ Type: issueTypeWithIntent{ Value: issueType, @@ -786,7 +791,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser Suggest: isSuggestion, }, } - } else { + default: body = &github.UpdateIssueRequest{Type: &issueType} } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3e0974862e..101cd21dc5 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "maps" "net/http" "strings" "sync/atomic" @@ -1434,6 +1435,7 @@ func Test_CreateIssue(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "labels") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "milestone") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "type") + assert.Equal(t, "string", tool.InputSchema.(*jsonschema.Schema).Properties["type"].Type) assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "issue_fields") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"method", "owner", "repo"}) @@ -1512,6 +1514,36 @@ func Test_CreateIssue(t *testing.T) { State: github.Ptr("open"), }, }, + { + name: "empty issue type is omitted when creating", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "title": "Issue without a type", + "body": "", + }).andThen( + mockResponse(t, http.StatusCreated, &github.Issue{ + Number: github.Ptr(125), + Title: github.Ptr("Issue without a type"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"), + State: github.Ptr("open"), + }), + ), + }), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Issue without a type", + "type": "", + }, + expectError: false, + expectedIssue: &github.Issue{ + Number: github.Ptr(125), + Title: github.Ptr("Issue without a type"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/125"), + State: github.Ptr("open"), + }, + }, { name: "successful issue creation with issue fields reconciled by names", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -2809,6 +2841,190 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { }) } +func TestIssueWriteUpdatesIssueType(t *testing.T) { + tests := []struct { + name string + args map[string]any + wantRequestBody string + }{ + { + name: "omit issue type", + args: map[string]any{ + "title": "Updated title", + }, + wantRequestBody: `{"title":"Updated title"}`, + }, + { + name: "set issue type", + args: map[string]any{ + "type": "Bug", + }, + wantRequestBody: `{"type":"Bug"}`, + }, + { + name: "clear issue type", + args: map[string]any{ + "type": "", + }, + wantRequestBody: `{"type":null}`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var gotRequestBody []byte + var readErr error + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + gotRequestBody, readErr = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"number":123,"html_url":"https://github.com/owner/repo/issues/123"}`)) + }, + })) + deps := BaseDeps{ + Client: client, + GQLClient: githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), + } + serverTool := IssueWrite(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + requestArgs := map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + } + maps.Copy(requestArgs, tc.args) + request := createMCPRequest(requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + require.NoError(t, readErr) + require.JSONEq(t, tc.wantRequestBody, string(gotRequestBody)) + }) + } +} + +func TestIssueWriteClearTypeBypassesMCPAppForm(t *testing.T) { + var gotRequestBody []byte + var readErr error + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + gotRequestBody, readErr = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"number":123,"html_url":"https://github.com/owner/repo/issues/123"}`)) + }, + })) + deps := BaseDeps{ + Client: client, + GQLClient: githubv4.NewClient(githubv4mock.NewMockedHTTPClient()), + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), + } + serverTool := IssueWrite(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "type": "", + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + require.NoError(t, readErr) + require.JSONEq(t, `{"type":null}`, string(gotRequestBody)) + textContent := getTextResult(t, result) + require.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/123") +} + +func TestIssueWriteClearTypeBypassesMCPAppFormWithStateChange(t *testing.T) { + var gotRequestBody []byte + var readErr error + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + gotRequestBody, readErr = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"number":123,"html_url":"https://github.com/owner/repo/issues/123"}`)) + }, + })) + issueID := githubv4.ID("I_kwDOA0xdyM50BPaO") + stateReason := IssueClosedStateReasonCompleted + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + 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(123), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{"id": issueID}, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"closeIssue(input: $input)"` + }{}, + CloseIssueInput{ + IssueID: issueID, + StateReason: &stateReason, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "closeIssue": map[string]any{ + "issue": map[string]any{ + "id": issueID, + "number": 123, + "url": "https://github.com/owner/repo/issues/123", + "state": "CLOSED", + }, + }, + }), + ), + )) + deps := BaseDeps{ + Client: client, + GQLClient: gqlClient, + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), + } + serverTool := IssueWrite(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + request := createMCPRequestWithSession(t, ClientNameVSCodeInsiders, true, map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "type": "", + "state": "closed", + "state_reason": "completed", + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + require.NoError(t, readErr) + require.JSONEq(t, `{"type":null}`, string(gotRequestBody)) + textContent := getTextResult(t, result) + require.Contains(t, textContent.Text, "https://github.com/owner/repo/issues/123") +} + func Test_UpdateIssue(t *testing.T) { // Verify tool definition serverTool := IssueWrite(translations.NullTranslationHelper)