diff --git a/README.md b/README.md index f83e108d81..f3114fb900 100644 --- a/README.md +++ b/README.md @@ -926,7 +926,7 @@ The following sets of tools are available: - **Required OAuth Scopes**: `repo` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'. (number, optional) - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) @@ -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, pass null 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 | null, optional) - **list_issue_fields** - List issue fields - **Required OAuth Scopes (any of)**: `repo`, `read:org` diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 212851c50d..a2310e2106 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -273,19 +273,7 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { requiredStr = "required" } - var typeStr string - - // Get the type and description - switch prop.Type { - case "array": - if prop.Items != nil { - typeStr = prop.Items.Type + "[]" - } else { - typeStr = "array" - } - default: - typeStr = prop.Type - } + typeStr := schemaTypeString(prop) // Indent any continuation lines in the description to maintain markdown formatting description := indentMultilineDescription(prop.Description, " ") @@ -300,6 +288,40 @@ func writeToolDoc(buf *strings.Builder, tool inventory.ServerTool) { } } +func schemaTypeString(schema *jsonschema.Schema) string { + switch { + case schema.Type == "array": + if schema.Items != nil { + return schema.Items.Type + "[]" + } + return "array" + case schema.Type != "": + return schema.Type + case len(schema.Types) > 0: + return strings.Join(schema.Types, " | ") + } + + var union []*jsonschema.Schema + switch { + case len(schema.AnyOf) > 0: + union = schema.AnyOf + case len(schema.OneOf) > 0: + union = schema.OneOf + default: + // A schema without type constraints accepts any value. + return "any" + } + + types := make([]string, 0, len(union)) + for _, member := range union { + memberType := schemaTypeString(member) + if !slices.Contains(types, memberType) { + types = append(types, memberType) + } + } + return strings.Join(types, " | ") +} + // scopesEqual checks if two scope slices contain the same elements (order-independent) func scopesEqual(a, b []string) bool { if len(a) != len(b) { diff --git a/cmd/github-mcp-server/main_test.go b/cmd/github-mcp-server/main_test.go index 476f308721..aa81c637dd 100644 --- a/cmd/github-mcp-server/main_test.go +++ b/cmd/github-mcp-server/main_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/google/jsonschema-go/jsonschema" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -36,3 +37,29 @@ func TestGitHubAppFlagsAreStdioOnly(t *testing.T) { assert.NotNil(t, stdioCmd.Flags().Lookup("app-id")) assert.Nil(t, httpCmd.Flags().Lookup("app-id")) } + +func TestSchemaTypeString(t *testing.T) { + tests := []struct { + name string + schema *jsonschema.Schema + want string + }{ + {name: "type", schema: &jsonschema.Schema{Type: "string"}, want: "string"}, + {name: "types", schema: &jsonschema.Schema{Types: []string{"string", "number"}}, want: "string | number"}, + {name: "unconstrained", schema: &jsonschema.Schema{}, want: "any"}, + {name: "anyOf", schema: &jsonschema.Schema{AnyOf: []*jsonschema.Schema{{Type: "string"}, {Type: "null"}}}, want: "string | null"}, + {name: "oneOf", schema: &jsonschema.Schema{OneOf: []*jsonschema.Schema{{Type: "number"}, {Type: "string"}}}, want: "number | string"}, + { + name: "array", + schema: &jsonschema.Schema{Type: "array", Items: &jsonschema.Schema{Type: "string"}}, + want: "string[]", + }, + {name: "untyped array", schema: &jsonschema.Schema{Type: "array"}, want: "array"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, schemaTypeString(tc.schema)) + }) + } +} diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 32891af62e..1f78e3580b 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -56,7 +56,7 @@ runtime behavior (such as output formatting) won't appear here. - **MCP App UI**: `ui://github-mcp-server/issue-write` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'. (number, optional) - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) @@ -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, pass null 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 | null, 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 null to remove the current type (string | null, 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..350522bf5e 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -50,7 +50,7 @@ The list below is generated from the Go source. It covers tool **inventory and s - **MCP App UI**: `ui://github-mcp-server/issue-write` - `assignees`: Usernames to assign to this issue (string[], optional) - `body`: Issue body content (string, optional) - - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'. (number, optional) - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'. (object[], optional) - `issue_number`: Issue number to update (number, optional) - `labels`: Labels to apply to this issue (string[], optional) @@ -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, pass null 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 | null, 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..10efb6c6df 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -28,7 +28,7 @@ "type": "string" }, "duplicate_of": { - "description": "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + "description": "Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'.", "type": "number" }, "issue_fields": { @@ -120,8 +120,16 @@ "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.", - "type": "string" + "anyOf": [ + { + "minLength": 1, + "type": "string" + }, + { + "type": "null" + } + ], + "description": "Type of this issue. For updates, pass null 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." } }, "required": [ diff --git a/pkg/github/__toolsnaps__/update_issue_type.snap b/pkg/github/__toolsnaps__/update_issue_type.snap index 21a2f64bd5..fbe8c90bb1 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. Pass null 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,8 +28,16 @@ "type": "number" }, "issue_type": { - "description": "The issue type to set", - "type": "string" + "anyOf": [ + { + "minLength": 1, + "type": "string" + }, + { + "type": "null" + } + ], + "description": "The issue type to set, or null to remove the current type" }, "owner": { "description": "Repository owner (username or organization)", diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 58fd904e88..ae04045a48 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -3,6 +3,7 @@ package github import ( "context" "encoding/json" + "maps" "net/http" "strings" "testing" @@ -787,6 +788,18 @@ func TestGranularUpdateIssueType(t *testing.T) { }, }, }, + { + name: "remove type with null", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "issue_type": nil, + }, + expectedReq: map[string]any{ + "type": nil, + }, + }, } for _, tc := range tests { @@ -807,6 +820,45 @@ func TestGranularUpdateIssueType(t *testing.T) { } } +func TestGranularUpdateIssueTypeRejectsInvalidInput(t *testing.T) { + tests := []struct { + name string + args map[string]any + omitType bool + wantError string + }{ + {name: "missing type", omitType: true, wantError: "missing required parameter: issue_type"}, + {name: "empty type", args: map[string]any{"issue_type": ""}, wantError: "parameter issue_type must not be empty"}, + {name: "null with rationale", args: map[string]any{"rationale": "live validation"}, wantError: "suggestion metadata is not supported"}, + {name: "null with confidence", args: map[string]any{"confidence": "HIGH"}, wantError: "suggestion metadata is not supported"}, + {name: "null suggestion", args: map[string]any{"is_suggestion": true}, wantError: "suggestion metadata is not supported"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps := BaseDeps{} + serverTool := GranularUpdateIssueType(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + args := map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(1), + "issue_type": nil, + } + if tc.omitType { + delete(args, "issue_type") + } + maps.Copy(args, tc.args) + request := createMCPRequest(args) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.wantError) + }) + } +} + func TestGranularUpdateIssueTypeSuggest(t *testing.T) { tests := []struct { name string diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0577737e3d..dfb823e26b 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2240,8 +2240,11 @@ Options are: Description: "Milestone number", }, "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.", + AnyOf: []*jsonschema.Schema{ + {Type: "string", MinLength: jsonschema.Ptr(1)}, + {Type: "null"}, + }, + Description: "Type of this issue. For updates, pass null 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", @@ -2255,7 +2258,7 @@ Options are: }, "duplicate_of": { Type: "number", - Description: "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + Description: "Issue number that this issue is a duplicate of. Required when state_reason is 'duplicate'.", }, "issue_fields": { Type: "array", @@ -2365,10 +2368,14 @@ Options are: } // Get optional type - issueType, err := OptionalParam[string](args, "type") + issueTypeParam, issueTypeProvided, err := OptionalNullableStringParam(args, "type") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + issueType := "" + if issueTypeParam != nil { + issueType = *issueTypeParam + } // Handle state, state_reason and duplicateOf parameters state, err := OptionalParam[string](args, "state") @@ -2388,6 +2395,9 @@ Options are: if duplicateOf != 0 && stateReason != "duplicate" { return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil } + if err := validateDuplicateState(state, stateReason, duplicateOf); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } var issueFields []issueWriteFieldInput issueFields, err = optionalIssueWriteFields(args) @@ -2426,6 +2436,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: @@ -2496,9 +2507,16 @@ 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. + 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) { + // UpdateIssue is exported and may be called without the tool handler. + if err := validateDuplicateState(state, stateReason, duplicateOf); err != nil { + return utils.NewToolResultError(err.Error()), nil + } + updateOptions := UpdateIssueOptions{ AssigneesProvided: len(assignees) > 0, LabelsProvided: len(labels) > 0, @@ -2506,6 +2524,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 @@ -2579,7 +2598,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", @@ -2636,11 +2655,6 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 // Use GraphQL API for state updates if state != "" { - // Mandate specifying duplicateOf when trying to close as duplicate - if state == "closed" && stateReason == "duplicate" && duplicateOf == 0 { - return utils.NewToolResultError("duplicate_of must be provided when state_reason is 'duplicate'"), nil - } - // Get target issue ID (and duplicate issue ID if needed) issueID, duplicateIssueID, err := fetchIssueIDs(ctx, gqlClient, owner, repo, issueNumber, duplicateOf) if err != nil { @@ -2712,6 +2726,35 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return utils.NewToolResultText(string(r)), nil } +func validateDuplicateState(state, stateReason string, duplicateOf int) error { + if state == "closed" && stateReason == "duplicate" && duplicateOf == 0 { + return fmt.Errorf("duplicate_of must be provided when state_reason is 'duplicate'") + } + return 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..e05eda2b1d 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. Pass null 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, @@ -709,8 +709,11 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser Minimum: jsonschema.Ptr(1.0), }, "issue_type": { - Type: "string", - Description: "The issue type to set", + AnyOf: []*jsonschema.Schema{ + {Type: "string", MinLength: jsonschema.Ptr(1)}, + {Type: "null"}, + }, + Description: "The issue type to set, or null to remove the current type", }, "rationale": { Type: "string", @@ -746,10 +749,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 := OptionalNullableStringParam(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,24 +776,29 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - + if issueType == nil && (rationale != "" || confidence != "" || isSuggestion) { + return utils.NewToolResultError("suggestion metadata is not supported when removing an issue type; omit rationale, confidence, and is_suggestion"), 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 == nil: + body = map[string]any{"type": nil} + case rationale != "" || isSuggestion || confidence != "": body = &issueTypeUpdateRequest{ Type: issueTypeWithIntent{ - Value: issueType, + Value: *issueType, Rationale: rationale, Confidence: confidence, Suggest: isSuggestion, }, } - } else { - body = &github.UpdateIssueRequest{Type: &issueType} + default: + body = &github.UpdateIssueRequest{Type: issueType} } apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index ab271883b9..77380e5e21 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" @@ -3062,6 +3063,70 @@ 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": nil, + }, + 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 Test_UpdateIssue(t *testing.T) { // Verify tool definition serverTool := IssueWrite(translations.NullTranslationHelper) @@ -3172,6 +3237,7 @@ func Test_UpdateIssue(t *testing.T) { expectError bool expectedIssue *github.Issue expectedErrMsg string + expectNoRequests bool }{ { name: "partial update of non-state fields only", @@ -3591,11 +3657,35 @@ func Test_UpdateIssue(t *testing.T) { expectError: true, expectedErrMsg: "duplicate_of can only be used when state_reason is 'duplicate'", }, + { + name: "duplicate state reason without duplicate_of should fail before updates", + mockedRESTClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "type": nil, + "state": "closed", + "state_reason": "duplicate", + }, + expectError: true, + expectedErrMsg: "duplicate_of must be provided when state_reason is 'duplicate'", + expectNoRequests: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Setup clients with mocks + var restRequests, gqlRequests *requestCountingTransport + if tc.expectNoRequests { + restRequests = &requestCountingTransport{inner: tc.mockedRESTClient.Transport} + tc.mockedRESTClient.Transport = restRequests + gqlRequests = &requestCountingTransport{inner: tc.mockedGQLClient.Transport} + tc.mockedGQLClient.Transport = gqlRequests + } restClient := mustNewGHClient(t, tc.mockedRESTClient) gqlClient := githubv4.NewClient(tc.mockedGQLClient) deps := BaseDeps{ @@ -3609,6 +3699,10 @@ func Test_UpdateIssue(t *testing.T) { // Call handler result, err := handler(ContextWithDeps(context.Background(), deps), &request) + if tc.expectNoRequests { + assert.Zero(t, restRequests.count) + assert.Zero(t, gqlRequests.count) + } // Verify results if tc.expectError || tc.expectedErrMsg != "" { diff --git a/pkg/github/params.go b/pkg/github/params.go index 9be51b94b9..a03a6a1581 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -34,6 +34,26 @@ func OptionalParamOK[T any, A map[string]any](args A, p string) (value T, ok boo return } +// OptionalNullableStringParam preserves omitted, null, and non-empty string values. +func OptionalNullableStringParam(args map[string]any, p string) (*string, bool, error) { + value, ok := args[p] + if !ok { + return nil, false, nil + } + if value == nil { + return nil, true, nil + } + + stringValue, ok := value.(string) + if !ok { + return nil, true, fmt.Errorf("parameter %s is not of type string or null, is %T", p, value) + } + if stringValue == "" { + return nil, true, fmt.Errorf("parameter %s must not be empty", p) + } + return &stringValue, true, nil +} + // isAcceptedError checks if the error is an accepted error. func isAcceptedError(err error) bool { var acceptedError *github.AcceptedError diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index cbac37fee5..55a5526f77 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/go-github/v89/github" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_IsAcceptedError(t *testing.T) { @@ -149,6 +150,40 @@ func Test_OptionalStringParam(t *testing.T) { } } +func TestOptionalNullableStringParam(t *testing.T) { + tests := []struct { + name string + params map[string]any + want string + wantProvided bool + wantError string + }{ + {name: "omitted", params: map[string]any{}}, + {name: "null", params: map[string]any{"type": nil}, wantProvided: true}, + {name: "string", params: map[string]any{"type": "Bug"}, want: "Bug", wantProvided: true}, + {name: "empty", params: map[string]any{"type": ""}, wantProvided: true, wantError: "parameter type must not be empty"}, + {name: "wrong type", params: map[string]any{"type": float64(1)}, wantProvided: true, wantError: "parameter type is not of type string or null, is float64"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, provided, err := OptionalNullableStringParam(tc.params, "type") + + assert.Equal(t, tc.wantProvided, provided) + if tc.wantError != "" { + require.EqualError(t, err, tc.wantError) + return + } + require.NoError(t, err) + if tc.want == "" { + assert.Nil(t, got) + } else { + assert.Equal(t, tc.want, *got) + } + }) + } +} + func Test_RequiredInt(t *testing.T) { tests := []struct { name string diff --git a/ui/src/apps/issue-write/App.tsx b/ui/src/apps/issue-write/App.tsx index 95a549f28b..ee48ea5d33 100644 --- a/ui/src/apps/issue-write/App.tsx +++ b/ui/src/apps/issue-write/App.tsx @@ -418,6 +418,7 @@ function CreateIssueApp() { // Issue types state const [availableIssueTypes, setAvailableIssueTypes] = useState([]); const [selectedIssueType, setSelectedIssueType] = useState(null); + const [issueTypeCleared, setIssueTypeCleared] = useState(false); const [issueTypesLoading, setIssueTypesLoading] = useState(false); // State transition state @@ -721,7 +722,11 @@ function CreateIssueApp() { setSelectedLabels([]); setSelectedAssignees([]); setSelectedMilestone(null); - setSelectedIssueType(null); + const inputIssueType = toolInput?.type; + setSelectedIssueType( + typeof inputIssueType === "string" ? { id: inputIssueType, text: inputIssueType } : null + ); + setIssueTypeCleared(inputIssueType === null); setCurrentState("open"); setStateReason("completed"); setDuplicateOf(""); @@ -800,7 +805,7 @@ function CreateIssueApp() { // Pre-fill issue type immediately from issue data const issueTypeName = issueData.type?.name || (typeof issueData.type === 'string' ? issueData.type : null); - if (issueTypeName && !prefillApplied.current.type) { + if (issueTypeName && toolInput?.type === undefined && !prefillApplied.current.type) { setSelectedIssueType({ id: issueTypeName, text: issueTypeName }); prefillApplied.current.type = true; } @@ -829,7 +834,7 @@ function CreateIssueApp() { }; loadExistingIssue(); - }, [isUpdateMode, owner, repo, issueNumber, app, callTool, existingIssueData]); + }, [isUpdateMode, owner, repo, issueNumber, app, callTool, existingIssueData, toolInput]); // Apply existing labels when available labels load useEffect(() => { @@ -1016,6 +1021,7 @@ function CreateIssueApp() { delete params.state_reason; delete params.duplicate_of; delete params.issue_fields; + delete params.type; if (isUpdateMode && issueNumber) { params.issue_number = issueNumber; @@ -1032,6 +1038,8 @@ function CreateIssueApp() { } if (selectedIssueType) { params.type = selectedIssueType.text; + } else if (issueTypeCleared) { + params.type = null; } if (requestedState) { @@ -1115,6 +1123,7 @@ function CreateIssueApp() { selectedAssignees, selectedMilestone, selectedIssueType, + issueTypeCleared, isUpdateMode, issueNumber, stateReason, @@ -1533,7 +1542,11 @@ function CreateIssueApp() { <> {selectedIssueType && ( setSelectedIssueType(null)} + onSelect={() => { + setSelectedIssueType(null); + setIssueTypeCleared(true); + prefillApplied.current.type = true; + }} > Clear selection @@ -1542,7 +1555,11 @@ function CreateIssueApp() { setSelectedIssueType(type)} + onSelect={() => { + setSelectedIssueType(type); + setIssueTypeCleared(false); + prefillApplied.current.type = true; + }} > {type.text}