diff --git a/README.md b/README.md index 9527478a0f..2e6d94b84b 100644 --- a/README.md +++ b/README.md @@ -937,6 +937,9 @@ The following sets of tools are available: (string, required) - `milestone`: Milestone number (number, optional) - `owner`: Repository owner (string, required) + - `parent_issue_number`: Issue number of the parent issue. Only used when method is 'create' and cannot be combined with issue_fields. The new issue is created and attached to this parent in the same operation. (number, optional) + - `parent_owner`: Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) + - `parent_repo`: Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) - `repo`: Repository name (string, required) - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0de5bdd722..0535631481 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -67,6 +67,9 @@ runtime behavior (such as output formatting) won't appear here. (string, required) - `milestone`: Milestone number (number, optional) - `owner`: Repository owner (string, required) + - `parent_issue_number`: Issue number of the parent issue. Only used when method is 'create' and cannot be combined with issue_fields. The new issue is created and attached to this parent in the same operation. (number, optional) + - `parent_owner`: Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) + - `parent_repo`: Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) - `repo`: Repository name (string, required) - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) @@ -122,6 +125,9 @@ runtime behavior (such as output formatting) won't appear here. - **Required OAuth Scopes**: `repo` - `body`: Issue body content (optional) (string, optional) - `owner`: Repository owner (username or organization) (string, required) + - `parent_issue_number`: Issue number of the parent issue. The new issue is created and attached to this parent in the same operation. (number, optional) + - `parent_owner`: Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when parent_issue_number is provided. (string, optional) + - `parent_repo`: Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when parent_issue_number is provided. (string, optional) - `repo`: Repository name (string, required) - `title`: Issue title (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 350522bf5e..c6b1844045 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -61,6 +61,9 @@ The list below is generated from the Go source. It covers tool **inventory and s (string, required) - `milestone`: Milestone number (number, optional) - `owner`: Repository owner (string, required) + - `parent_issue_number`: Issue number of the parent issue. Only used when method is 'create' and cannot be combined with issue_fields. The new issue is created and attached to this parent in the same operation. (number, optional) + - `parent_owner`: Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) + - `parent_repo`: Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided. (string, optional) - `repo`: Repository name (string, required) - `state`: New state (string, optional) - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) diff --git a/pkg/github/__toolsnaps__/create_issue.snap b/pkg/github/__toolsnaps__/create_issue.snap index e0963741ac..d7de241265 100644 --- a/pkg/github/__toolsnaps__/create_issue.snap +++ b/pkg/github/__toolsnaps__/create_issue.snap @@ -17,6 +17,19 @@ "description": "Repository owner (username or organization)", "type": "string" }, + "parent_issue_number": { + "description": "Issue number of the parent issue. The new issue is created and attached to this parent in the same operation.", + "minimum": 1, + "type": "number" + }, + "parent_owner": { + "description": "Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when parent_issue_number is provided.", + "type": "string" + }, + "parent_repo": { + "description": "Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when parent_issue_number is provided.", + "type": "string" + }, "repo": { "description": "Repository name", "type": "string" diff --git a/pkg/github/__toolsnaps__/issue_write.snap b/pkg/github/__toolsnaps__/issue_write.snap index d4968c4f2f..20cc730f51 100644 --- a/pkg/github/__toolsnaps__/issue_write.snap +++ b/pkg/github/__toolsnaps__/issue_write.snap @@ -91,6 +91,19 @@ "description": "Repository owner", "type": "string" }, + "parent_issue_number": { + "description": "Issue number of the parent issue. Only used when method is 'create' and cannot be combined with issue_fields. The new issue is created and attached to this parent in the same operation.", + "minimum": 1, + "type": "number" + }, + "parent_owner": { + "description": "Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided.", + "type": "string" + }, + "parent_repo": { + "description": "Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided.", + "type": "string" + }, "repo": { "description": "Repository name", "type": "string" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index d6cc55e1ed..5767d2ec96 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -3,6 +3,7 @@ package github import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -2326,11 +2327,9 @@ const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write" // issueWriteFormParams are the parameters the issue_write MCP App form collects // and re-sends on submit. Any other parameter present on a call cannot be -// represented by the form. The form collects (and prefills) every parameter in -// the tool's current input schema, so hasNonFormParams against this set is a -// forward-compatibility safety net: a parameter added to the schema in the -// future but not yet wired into the form trips the check and bypasses the form -// so the supplied value isn't silently dropped. +// represented by the form, so hasNonFormParams bypasses the form rather than +// silently dropping it. Parent issue parameters are intentionally omitted +// because the current form cannot represent them. var issueWriteFormParams = map[string]struct{}{ "method": {}, "owner": {}, @@ -2425,6 +2424,19 @@ Options are: Type: "number", Description: "Issue number to update", }, + "parent_issue_number": { + Type: "number", + Description: "Issue number of the parent issue. Only used when method is 'create' and cannot be combined with issue_fields. The new issue is created and attached to this parent in the same operation.", + Minimum: jsonschema.Ptr(1.0), + }, + "parent_owner": { + Type: "string", + Description: "Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided.", + }, + "parent_repo": { + Type: "string", + Description: "Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when method is 'create' and parent_issue_number is provided.", + }, "title": { Type: "string", Description: "Issue title", @@ -2611,11 +2623,38 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } + parentIssueNumber, err := OptionalIntParam(args, "parent_issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + parentValue, parentProvided := args["parent_issue_number"] + parentProvided = parentProvided && parentValue != nil + if parentProvided && parentIssueNumber < 1 { + return utils.NewToolResultError("parent_issue_number must be greater than 0"), nil, nil + } + if parentProvided && method != "create" { + return utils.NewToolResultError("parent_issue_number can only be used with the create method"), nil, nil + } + parentOwner, err := OptionalParam[string](args, "parent_owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + parentRepo, err := OptionalParam[string](args, "parent_repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if err := validateParentRepository(parentProvided, parentOwner, parentRepo); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + var issueFields []issueWriteFieldInput issueFields, err = optionalIssueWriteFields(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if parentProvided && len(issueFields) > 0 { + return utils.NewToolResultError("issue_fields cannot be used with parent_issue_number"), nil, nil + } client, err := deps.GetClient(ctx) if err != nil { @@ -2638,6 +2677,11 @@ Options are: switch method { case "create": + if parentProvided { + result, err := createIssueWithParent(ctx, client, gqlClient, owner, repo, title, body, assignees, labels, milestoneNum, issueType, parentIssueNumber, parentOwner, parentRepo) + return result, nil, err + } + result, err := CreateIssue(ctx, client, owner, repo, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues) return result, nil, err case "update": @@ -2659,6 +2703,242 @@ Options are: return st } +type CreateIssueInput struct { + RepositoryID githubv4.ID `json:"repositoryId"` + Title githubv4.String `json:"title"` + + Body *githubv4.String `json:"body,omitempty"` + AssigneeIDs *[]githubv4.ID `json:"assigneeIds,omitempty"` + MilestoneID *githubv4.ID `json:"milestoneId,omitempty"` + LabelIDs *[]githubv4.ID `json:"labelIds,omitempty"` + IssueTypeID *githubv4.ID `json:"issueTypeId,omitempty"` + ParentIssueID *githubv4.ID `json:"parentIssueId,omitempty"` +} + +type createIssueMutation struct { + CreateIssue struct { + Issue struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + URL githubv4.URI + } + } `graphql:"createIssue(input: $input)"` +} + +type createIssueParentMetadataQuery struct { + ChildRepository struct { + ID githubv4.ID + NameWithOwner githubv4.String + } `graphql:"childRepository: repository(owner: $owner, name: $repo)"` + ParentRepository struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + } `graphql:"issue(number: $parentIssueNumber)"` + } `graphql:"parentRepository: repository(owner: $parentOwner, name: $parentRepo)"` +} + +func createIssueWithParent( + ctx context.Context, + client *github.Client, + gqlClient *githubv4.Client, + owner string, + repo string, + title string, + body string, + assignees []string, + labels []string, + milestoneNumber int, + issueType string, + parentIssueNumber int, + parentOwner string, + parentRepo string, +) (*mcp.CallToolResult, error) { + if title == "" { + return utils.NewToolResultError("missing required parameter: title"), nil + } + if parentIssueNumber < 1 { + return utils.NewToolResultError("parent_issue_number must be greater than 0"), nil + } + + parentOwner, parentRepo = parentRepository(owner, repo, parentOwner, parentRepo) + repositoryID, parentIssueID, err := resolveCreateIssueParent(ctx, gqlClient, owner, repo, parentOwner, parentRepo, parentIssueNumber) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to resolve parent issue", err), nil + } + + input := CreateIssueInput{ + RepositoryID: repositoryID, + Title: githubv4.String(title), + ParentIssueID: &parentIssueID, + } + if body != "" { + input.Body = githubv4.NewString(githubv4.String(body)) + } + + if len(labels) > 0 { + labelIDs := make([]githubv4.ID, 0, len(labels)) + for _, label := range labels { + labelID, err := getLabelID(ctx, gqlClient, owner, repo, label) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, fmt.Sprintf("failed to resolve label %q", label), err), nil + } + labelIDs = append(labelIDs, labelID) + } + input.LabelIDs = &labelIDs + } + + if len(assignees) > 0 { + assigneeIDs := make([]githubv4.ID, 0, len(assignees)) + for _, assignee := range assignees { + assigneeID, err := resolveUserID(ctx, gqlClient, assignee) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, fmt.Sprintf("failed to resolve assignee %q", assignee), err), nil + } + assigneeIDs = append(assigneeIDs, assigneeID) + } + input.AssigneeIDs = &assigneeIDs + } + + if milestoneNumber != 0 { + milestoneID, err := resolveMilestoneID(ctx, gqlClient, owner, repo, milestoneNumber) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to resolve milestone", err), nil + } + input.MilestoneID = &milestoneID + } + + if issueType != "" { + issueTypeID, resp, err := resolveIssueTypeID(ctx, client, owner, repo, issueType) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to resolve issue type %q", issueType), resp, err), nil + } + input.IssueTypeID = &issueTypeID + } + + var mutation createIssueMutation + if err := gqlClient.Mutate(ctx, &mutation, input, nil); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to create issue", err), nil + } + if mutation.CreateIssue.Issue.FullDatabaseID == "" || mutation.CreateIssue.Issue.URL.URL == nil { + return utils.NewToolResultError("failed to create issue: response did not include the created issue"), nil + } + + response := MinimalResponse{ + ID: string(mutation.CreateIssue.Issue.FullDatabaseID), + URL: mutation.CreateIssue.Issue.URL.String(), + } + encoded, err := json.Marshal(response) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil + } + return utils.NewToolResultText(string(encoded)), nil +} + +func parentRepository(owner, repo, parentOwner, parentRepo string) (string, string) { + if parentOwner == "" && parentRepo == "" { + return owner, repo + } + return parentOwner, parentRepo +} + +func validateParentRepository(parentProvided bool, parentOwner, parentRepo string) error { + if !parentProvided { + if parentOwner != "" || parentRepo != "" { + return errors.New("parent_owner and parent_repo can only be used when parent_issue_number is provided") + } + return nil + } + if (parentOwner == "") != (parentRepo == "") { + return errors.New("parent_owner and parent_repo must be provided together") + } + return nil +} + +func resolveCreateIssueParent(ctx context.Context, gqlClient *githubv4.Client, owner, repo, parentOwner, parentRepo string, parentIssueNumber int) (githubv4.ID, githubv4.ID, error) { + var query createIssueParentMetadataQuery + variables := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "parentOwner": githubv4.String(parentOwner), + "parentRepo": githubv4.String(parentRepo), + "parentIssueNumber": githubv4.Int(parentIssueNumber), // #nosec G115 - issue numbers are small positive integers + } + if err := gqlClient.Query(ctx, &query, variables); err != nil { + return "", "", err + } + if query.ChildRepository.NameWithOwner == "" { + return "", "", fmt.Errorf("repository %s/%s was not found", owner, repo) + } + if query.ParentRepository.Issue.Number == 0 { + return "", "", fmt.Errorf("parent issue #%d was not found in %s/%s", parentIssueNumber, parentOwner, parentRepo) + } + return query.ChildRepository.ID, query.ParentRepository.Issue.ID, nil +} + +func resolveUserID(ctx context.Context, gqlClient *githubv4.Client, login string) (githubv4.ID, error) { + var query struct { + User struct { + ID githubv4.ID + Login githubv4.String + } `graphql:"user(login: $login)"` + } + if err := gqlClient.Query(ctx, &query, map[string]any{"login": githubv4.String(login)}); err != nil { + return "", err + } + if query.User.Login == "" { + return "", fmt.Errorf("user %q was not found", login) + } + return query.User.ID, nil +} + +func resolveMilestoneID(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, milestoneNumber int) (githubv4.ID, error) { + var query struct { + Repository struct { + Milestone struct { + ID githubv4.ID + Number githubv4.Int + } `graphql:"milestone(number: $milestoneNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + variables := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "milestoneNumber": githubv4.Int(milestoneNumber), // #nosec G115 - milestone numbers are small positive integers + } + if err := gqlClient.Query(ctx, &query, variables); err != nil { + return "", err + } + if query.Repository.Milestone.Number == 0 { + return "", fmt.Errorf("milestone #%d was not found in %s/%s", milestoneNumber, owner, repo) + } + return query.Repository.Milestone.ID, nil +} + +func resolveIssueTypeID(ctx context.Context, client *github.Client, owner, repo, issueTypeName string) (githubv4.ID, *github.Response, error) { + req, err := client.NewRequest(ctx, "GET", fmt.Sprintf("repos/%s/%s/issue-types", owner, repo), nil) + if err != nil { + return "", nil, err + } + + var issueTypes []*github.IssueType + resp, err := client.Do(req, &issueTypes) + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + if err != nil { + return "", resp, err + } + for _, issueType := range issueTypes { + if issueType != nil && strings.EqualFold(strings.TrimSpace(issueType.GetName()), strings.TrimSpace(issueTypeName)) { + if issueType.GetNodeID() == "" { + return "", resp, fmt.Errorf("issue type %q is missing a node ID", issueTypeName) + } + return githubv4.ID(issueType.GetNodeID()), resp, nil + } + } + return "", resp, fmt.Errorf("issue type %q was not found in %s/%s", issueTypeName, owner, repo) +} + func CreateIssue(ctx context.Context, client *github.Client, owner string, repo string, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string, issueFieldValues []*github.IssueRequestFieldValue) (*mcp.CallToolResult, error) { if title == "" { return utils.NewToolResultError("missing required parameter: title"), nil diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index fb5ff32242..863ad89ab1 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -144,6 +144,19 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT Type: "string", Description: "Issue body content (optional)", }, + "parent_issue_number": { + Type: "number", + Description: "Issue number of the parent issue. The new issue is created and attached to this parent in the same operation.", + Minimum: jsonschema.Ptr(1.0), + }, + "parent_owner": { + Type: "string", + Description: "Repository owner of the parent issue. Must be provided with parent_repo. Omit both to use owner and repo. Only used when parent_issue_number is provided.", + }, + "parent_repo": { + Type: "string", + Description: "Repository name of the parent issue. Must be provided with parent_owner. Omit both to use owner and repo. Only used when parent_issue_number is provided.", + }, }, Required: []string{"owner", "repo", "title"}, }, @@ -163,6 +176,26 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT return utils.NewToolResultError(err.Error()), nil, nil } body, _ := OptionalParam[string](args, "body") + parentIssueNumber, err := OptionalIntParam(args, "parent_issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + parentValue, parentProvided := args["parent_issue_number"] + parentProvided = parentProvided && parentValue != nil + if parentProvided && parentIssueNumber < 1 { + return utils.NewToolResultError("parent_issue_number must be greater than 0"), nil, nil + } + parentOwner, err := OptionalParam[string](args, "parent_owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + parentRepo, err := OptionalParam[string](args, "parent_repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if err := validateParentRepository(parentProvided, parentOwner, parentRepo); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } issueReq := github.CreateIssueRequest{ Title: title, @@ -176,6 +209,15 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } + if parentProvided { + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + } + result, err := createIssueWithParent(ctx, client, gqlClient, owner, repo, title, body, nil, nil, 0, "", parentIssueNumber, parentOwner, parentRepo) + return result, nil, err + } + issue, resp, err := client.Issues.Create(ctx, owner, repo, issueReq) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create issue", resp, err), nil, nil diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 7e9fb2ee8e..bf024b545a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -22,6 +22,7 @@ import ( "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2240,6 +2241,713 @@ func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { }) } +func TestIssueWriteCreateWithParentAndLabelsUsesSingleMutation(t *testing.T) { + serverTool := IssueWrite(translations.NullTranslationHelper) + schema := serverTool.Tool.InputSchema + issueWriteSchema := schema.(*jsonschema.Schema) + assert.Contains(t, issueWriteSchema.Properties, "parent_issue_number") + assert.Contains(t, issueWriteSchema.Properties, "parent_owner") + assert.Contains(t, issueWriteSchema.Properties, "parent_repo") + assert.NotContains(t, issueWriteSchema.Required, "parent_issue_number") + assert.NotContains(t, issueWriteSchema.Required, "parent_owner") + assert.NotContains(t, issueWriteSchema.Required, "parent_repo") + + labelIDs := []githubv4.ID{"LABEL_backlog"} + parentID := githubv4.ID("ISSUE_parent") + expectedInput := CreateIssueInput{ + RepositoryID: githubv4.ID("REPO_1"), + Title: githubv4.String("Atomic child"), + Body: githubv4.NewString(githubv4.String("Created under its parent")), + LabelIDs: &labelIDs, + ParentIssueID: &parentID, + } + createMatcher := githubv4mock.NewMutationMatcher( + createIssueMutation{}, + expectedInput, + nil, + githubv4mock.DataResponse(map[string]any{ + "createIssue": map[string]any{ + "issue": map[string]any{ + "fullDatabaseId": "12345", + "url": "https://github.com/owner/repo/issues/2", + }, + }, + }), + ) + assert.Contains(t, createMatcher.Request, "$input:CreateIssueInput!") + + gqlHTTPClient, gqlCalls := countingGraphQLClient( + createIssueParentMatcher(1, "parent-owner", "parent-repo", "REPO_1", "ISSUE_parent"), + createIssueLabelMatcher("status:backlog", "LABEL_backlog"), + createMatcher, + ) + restHTTPClient := MockHTTPClientWithHandlers(nil) + restCounter := &countingRoundTripper{next: restHTTPClient.Transport} + restHTTPClient.Transport = restCounter + + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Atomic child", + "body": "Created under its parent", + "labels": []any{"status:backlog"}, + "parent_issue_number": float64(1), + "parent_owner": "parent-owner", + "parent_repo": "parent-repo", + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + assert.Equal(t, 3, gqlCalls(), "metadata lookups and exactly one create mutation are expected") + assert.Zero(t, restCounter.count.Load(), "parent creation must not use REST create or attachment requests") + + var response MinimalResponse + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, "12345", response.ID) + assert.Equal(t, "https://github.com/owner/repo/issues/2", response.URL) +} + +func TestIssueWriteCreateWithParentDoesNotFallbackAfterMutationFailure(t *testing.T) { + gqlHTTPClient, gqlCalls := countingGraphQLClient( + createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent"), + githubv4mock.NewMutationMatcher( + createIssueMutation{}, + CreateIssueInput{ + RepositoryID: githubv4.ID("REPO_1"), + Title: githubv4.String("Atomic child"), + ParentIssueID: githubv4mock.Ptr[githubv4.ID]("ISSUE_parent"), + }, + nil, + githubv4mock.ErrorResponse("parent cannot accept sub-issues"), + ), + ) + restHTTPClient := MockHTTPClientWithHandlers(nil) + restCounter := &countingRoundTripper{next: restHTTPClient.Transport} + restHTTPClient.Transport = restCounter + + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + serverTool := IssueWrite(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Atomic child", + "parent_issue_number": float64(7), + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, "failed to create issue") + assert.Equal(t, 2, gqlCalls(), "a failed create mutation must not trigger an attachment mutation") + assert.Zero(t, restCounter.count.Load(), "a failed create mutation must not fall back to REST create or attachment requests") +} + +func TestIssueWriteCreateWithParentRejectsIncompleteMutationResponse(t *testing.T) { + tests := []struct { + name string + data map[string]any + }{ + { + name: "missing issue", + data: map[string]any{"createIssue": map[string]any{"issue": nil}}, + }, + { + name: "missing database ID", + data: map[string]any{ + "createIssue": map[string]any{ + "issue": map[string]any{"url": "https://github.com/owner/repo/issues/8"}, + }, + }, + }, + { + name: "missing URL", + data: map[string]any{ + "createIssue": map[string]any{ + "issue": map[string]any{"fullDatabaseId": "34567"}, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gqlHTTPClient, gqlCalls := countingGraphQLClient( + createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent"), + githubv4mock.NewMutationMatcher( + createIssueMutation{}, + CreateIssueInput{ + RepositoryID: githubv4.ID("REPO_1"), + Title: githubv4.String("Atomic child"), + ParentIssueID: githubv4mock.Ptr[githubv4.ID]("ISSUE_parent"), + }, + nil, + githubv4mock.DataResponse(test.data), + ), + ) + restHTTPClient := MockHTTPClientWithHandlers(nil) + restCounter := &countingRoundTripper{next: restHTTPClient.Transport} + restHTTPClient.Transport = restCounter + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + request := createMCPRequest(map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Atomic child", + "parent_issue_number": float64(7), + }) + + serverTool := IssueWrite(translations.NullTranslationHelper) + result, err := serverTool.Handler(deps)( + ContextWithDeps(context.Background(), deps), + &request, + ) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, "response did not include the created issue") + assert.Equal(t, 2, gqlCalls()) + assert.Zero(t, restCounter.count.Load()) + }) + } +} + +func TestIssueWriteCreateWithParentPreservesSupportedFields(t *testing.T) { + labelIDs := []githubv4.ID{"LABEL_bug"} + assigneeIDs := []githubv4.ID{"USER_octocat"} + milestoneID := githubv4.ID("MILESTONE_1") + issueTypeID := githubv4.ID("ISSUE_TYPE_bug") + parentID := githubv4.ID("ISSUE_parent") + expectedInput := CreateIssueInput{ + RepositoryID: githubv4.ID("REPO_1"), + Title: githubv4.String("Fully specified child"), + Body: githubv4.NewString(githubv4.String("Body")), + AssigneeIDs: &assigneeIDs, + MilestoneID: &milestoneID, + LabelIDs: &labelIDs, + IssueTypeID: &issueTypeID, + ParentIssueID: &parentID, + } + gqlHTTPClient, gqlCalls := countingGraphQLClient( + createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent"), + createIssueLabelMatcher("bug", "LABEL_bug"), + createIssueUserMatcher("octocat", "USER_octocat"), + createIssueMilestoneMatcher(1, "MILESTONE_1"), + githubv4mock.NewMutationMatcher( + createIssueMutation{}, + expectedInput, + nil, + githubv4mock.DataResponse(map[string]any{ + "createIssue": map[string]any{ + "issue": map[string]any{ + "fullDatabaseId": "34567", + "url": "https://github.com/owner/repo/issues/8", + }, + }, + }), + ), + ) + restHTTPClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/issue-types": mockResponse(t, http.StatusOK, []*github.IssueType{ + {Name: github.Ptr("Bug"), NodeID: github.Ptr("ISSUE_TYPE_bug")}, + }), + }) + restCounter := &countingRoundTripper{next: restHTTPClient.Transport} + restHTTPClient.Transport = restCounter + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + request := createMCPRequest(map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Fully specified child", + "body": "Body", + "assignees": []any{"octocat"}, + "labels": []any{"bug"}, + "milestone": float64(1), + "type": "Bug", + "parent_issue_number": float64(7), + }) + + serverTool := IssueWrite(translations.NullTranslationHelper) + result, err := serverTool.Handler(deps)( + ContextWithDeps(context.Background(), deps), + &request, + ) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + assert.Equal(t, 5, gqlCalls(), "four metadata lookups and exactly one create mutation are expected") + assert.Equal(t, int64(1), restCounter.count.Load(), "issue type resolution is the only expected REST call") +} + +func TestIssueWriteCreateWithParentRejectsMissingMetadata(t *testing.T) { + tests := []struct { + name string + args map[string]any + gqlMatchers []githubv4mock.Matcher + restHandlers map[string]http.HandlerFunc + want string + wantGQLCalls int + wantRESTCalls int64 + }{ + { + name: "child repository", + gqlMatchers: []githubv4mock.Matcher{ + createIssueMissingChildRepositoryMatcher(7), + }, + want: "failed to resolve parent issue", + wantGQLCalls: 1, + }, + { + name: "parent issue", + gqlMatchers: []githubv4mock.Matcher{ + createIssueMissingParentMatcher(7), + }, + want: "failed to resolve parent issue", + wantGQLCalls: 1, + }, + { + name: "milestone", + args: map[string]any{"milestone": float64(99)}, + gqlMatchers: []githubv4mock.Matcher{ + createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent"), + createIssueMissingMilestoneMatcher(99), + }, + want: "failed to resolve milestone", + wantGQLCalls: 2, + }, + { + name: "assignee", + args: map[string]any{"assignees": []any{"missing-user"}}, + gqlMatchers: []githubv4mock.Matcher{ + createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent"), + createIssueMissingUserMatcher("missing-user"), + }, + want: `failed to resolve assignee "missing-user"`, + wantGQLCalls: 2, + }, + { + name: "issue type", + args: map[string]any{"type": "Missing"}, + gqlMatchers: []githubv4mock.Matcher{createIssueParentMatcher(7, "owner", "repo", "REPO_1", "ISSUE_parent")}, + restHandlers: map[string]http.HandlerFunc{ + "GET /repos/{owner}/{repo}/issue-types": mockResponse(t, http.StatusOK, []*github.IssueType{}), + }, + want: `failed to resolve issue type "Missing"`, + wantGQLCalls: 1, + wantRESTCalls: 1, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gqlHTTPClient, gqlCalls := countingGraphQLClient(test.gqlMatchers...) + restHTTPClient := MockHTTPClientWithHandlers(test.restHandlers) + restCounter := &countingRoundTripper{next: restHTTPClient.Transport} + restHTTPClient.Transport = restCounter + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + args := map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Atomic child", + "parent_issue_number": float64(7), + } + maps.Copy(args, test.args) + request := createMCPRequest(args) + + serverTool := IssueWrite(translations.NullTranslationHelper) + result, err := serverTool.Handler(deps)( + ContextWithDeps(context.Background(), deps), + &request, + ) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, test.want) + assert.Equal(t, test.wantGQLCalls, gqlCalls()) + assert.Equal(t, test.wantRESTCalls, restCounter.count.Load()) + }) + } +} + +func TestGranularCreateIssueWithParentUsesAtomicMutation(t *testing.T) { + serverTool := GranularCreateIssue(translations.NullTranslationHelper) + schema := serverTool.Tool.InputSchema.(*jsonschema.Schema) + assert.Contains(t, schema.Properties, "parent_issue_number") + assert.Contains(t, schema.Properties, "parent_owner") + assert.Contains(t, schema.Properties, "parent_repo") + + parentID := githubv4.ID("ISSUE_parent") + gqlHTTPClient := githubv4mock.NewMockedHTTPClient( + createIssueParentMatcher(3, "owner", "repo", "REPO_1", "ISSUE_parent"), + githubv4mock.NewMutationMatcher( + createIssueMutation{}, + CreateIssueInput{ + RepositoryID: githubv4.ID("REPO_1"), + Title: githubv4.String("Granular child"), + ParentIssueID: &parentID, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "createIssue": map[string]any{ + "issue": map[string]any{ + "fullDatabaseId": "23456", + "url": "https://github.com/owner/repo/issues/4", + }, + }, + }), + ), + ) + restHTTPClient := MockHTTPClientWithHandlers(nil) + + deps := BaseDeps{ + Client: mustNewGHClient(t, restHTTPClient), + GQLClient: githubv4.NewClient(gqlHTTPClient), + } + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Granular child", + "parent_issue_number": float64(3), + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) +} + +func TestCreateIssueParentRepositoryValidation(t *testing.T) { + tests := []struct { + name string + handler func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) + args map[string]any + want string + }{ + { + name: "issue_write", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := IssueWrite(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_owner": "parent-owner", + }, + want: "can only be used when parent_issue_number is provided", + }, + { + name: "create_issue", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := GranularCreateIssue(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_repo": "parent-repo", + }, + want: "can only be used when parent_issue_number is provided", + }, + { + name: "issue_write requires parent repo with parent owner", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := IssueWrite(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_issue_number": float64(1), + "parent_owner": "parent-owner", + }, + want: "parent_owner and parent_repo must be provided together", + }, + { + name: "create_issue requires parent owner with parent repo", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := GranularCreateIssue(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_issue_number": float64(1), + "parent_repo": "parent-repo", + }, + want: "parent_owner and parent_repo must be provided together", + }, + { + name: "issue fields", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := IssueWrite(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_issue_number": float64(1), + "issue_fields": []any{ + map[string]any{"field_name": "Priority", "field_option_name": "High"}, + }, + }, + want: "issue_fields cannot be used with parent_issue_number", + }, + { + name: "issue_write rejects parent during update", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := IssueWrite(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "method": "update", + "owner": "owner", + "repo": "repo", + "issue_number": float64(2), + "parent_issue_number": float64(1), + }, + want: "parent_issue_number can only be used with the create method", + }, + { + name: "issue_write rejects zero parent number", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := IssueWrite(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_issue_number": float64(0), + }, + want: "parent_issue_number must be greater than 0", + }, + { + name: "create_issue rejects zero parent number", + handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + serverTool := GranularCreateIssue(translations.NullTranslationHelper) + return serverTool.Handler(BaseDeps{})(ctx, request) + }, + args: map[string]any{ + "owner": "owner", + "repo": "repo", + "title": "Child", + "parent_issue_number": float64(0), + }, + want: "parent_issue_number must be greater than 0", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + request := createMCPRequest(test.args) + result, err := test.handler(ContextWithDeps(context.Background(), BaseDeps{}), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, test.want) + }) + } +} + +func createIssueParentMatcher(parentIssueNumber int, parentOwner, parentRepo string, repositoryID, parentIssueID githubv4.ID) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + createIssueParentMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "parentOwner": githubv4.String(parentOwner), + "parentRepo": githubv4.String(parentRepo), + "parentIssueNumber": githubv4.Int(parentIssueNumber), // #nosec G115 - test issue numbers are small + }, + githubv4mock.DataResponse(map[string]any{ + "childRepository": map[string]any{ + "id": repositoryID, + "nameWithOwner": "owner/repo", + }, + "parentRepository": map[string]any{ + "issue": map[string]any{ + "id": parentIssueID, + "number": parentIssueNumber, + }, + }, + }), + ) +} + +func createIssueLabelMatcher(name string, id githubv4.ID) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Label struct { + ID githubv4.ID + Name githubv4.String + } `graphql:"label(name: $name)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "name": githubv4.String(name), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "label": map[string]any{ + "id": id, + "name": name, + }, + }, + }), + ) +} + +func createIssueMissingChildRepositoryMatcher(parentIssueNumber int) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + createIssueParentMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "parentOwner": githubv4.String("owner"), + "parentRepo": githubv4.String("repo"), + "parentIssueNumber": githubv4.Int(parentIssueNumber), // #nosec G115 - test issue numbers are small + }, + githubv4mock.DataResponse(map[string]any{ + "childRepository": nil, + "parentRepository": map[string]any{ + "issue": map[string]any{ + "id": "ISSUE_parent", + "number": parentIssueNumber, + }, + }, + }), + ) +} + +func createIssueMissingParentMatcher(parentIssueNumber int) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + createIssueParentMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "parentOwner": githubv4.String("owner"), + "parentRepo": githubv4.String("repo"), + "parentIssueNumber": githubv4.Int(parentIssueNumber), // #nosec G115 - test issue numbers are small + }, + githubv4mock.DataResponse(map[string]any{ + "childRepository": map[string]any{ + "id": "REPO_1", + "nameWithOwner": "owner/repo", + }, + "parentRepository": map[string]any{"issue": nil}, + }), + ) +} + +func createIssueUserMatcher(login string, id githubv4.ID) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + User struct { + ID githubv4.ID + Login githubv4.String + } `graphql:"user(login: $login)"` + }{}, + map[string]any{"login": githubv4.String(login)}, + githubv4mock.DataResponse(map[string]any{ + "user": map[string]any{ + "id": id, + "login": login, + }, + }), + ) +} + +func createIssueMissingUserMatcher(login string) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + User struct { + ID githubv4.ID + Login githubv4.String + } `graphql:"user(login: $login)"` + }{}, + map[string]any{"login": githubv4.String(login)}, + githubv4mock.DataResponse(map[string]any{"user": nil}), + ) +} + +func createIssueMilestoneMatcher(number int, id githubv4.ID) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Milestone struct { + ID githubv4.ID + Number githubv4.Int + } `graphql:"milestone(number: $milestoneNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "milestoneNumber": githubv4.Int(number), // #nosec G115 - test milestone numbers are small + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "milestone": map[string]any{ + "id": id, + "number": number, + }, + }, + }), + ) +} + +func createIssueMissingMilestoneMatcher(number int) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Milestone struct { + ID githubv4.ID + Number githubv4.Int + } `graphql:"milestone(number: $milestoneNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "milestoneNumber": githubv4.Int(number), // #nosec G115 - test milestone numbers are small + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{"milestone": nil}, + }), + ) +} + func Test_issueWriteHasNonFormParams(t *testing.T) { t.Parallel() @@ -2258,6 +2966,9 @@ func Test_issueWriteHasNonFormParams(t *testing.T) { {name: "state present", args: map[string]any{"state": "closed"}, want: false}, {name: "state_reason present", args: map[string]any{"state_reason": "completed"}, want: false}, {name: "duplicate_of present", args: map[string]any{"duplicate_of": float64(7)}, want: false}, + {name: "parent issue present", args: map[string]any{"parent_issue_number": float64(7)}, want: true}, + {name: "parent owner present", args: map[string]any{"parent_owner": "octo-org"}, want: true}, + {name: "parent repo present", args: map[string]any{"parent_repo": "parent-repo"}, want: true}, {name: "unknown non-schema param present", args: map[string]any{"title": "t", "not_a_real_param": "x"}, want: true}, {name: "nil value is ignored", args: map[string]any{"issue_fields": nil}, want: false}, } @@ -2357,11 +3068,14 @@ func Test_issueWriteSchemaClassification(t *testing.T) { t.Parallel() // Schema properties the MCP App form cannot represent — their presence - // must trigger the safety-net bypass via hasNonFormParams. The - // form currently collects every schema property, so this allowlist is - // empty; add a property here only if it is added to the schema without + // must trigger the safety-net bypass via hasNonFormParams. Add a + // property here only if it is added to the schema without // corresponding form support. - knownNonForm := map[string]struct{}{} + knownNonForm := map[string]struct{}{ + "parent_issue_number": {}, + "parent_owner": {}, + "parent_repo": {}, + } cases := []struct { name string