From e65660588e1472777b36e32ebd79e7fd92b9240d Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Wed, 8 Jul 2026 11:17:14 +0100 Subject: [PATCH 1/2] Reimplement issue dependencies on go-github v89 REST API Issue dependencies (blocked_by / blocking) were implemented on GraphQL via githubv4. Because the pinned githubv4 library predates the dependency mutations, the code hand-declared AddBlockedByInput / RemoveBlockedByInput and resolved issue numbers to node IDs with a custom aliased query. go-github v89 adds first-class REST methods (ListBlockedBy, ListBlocking, AddBlockedBy, RemoveBlockedBy), so switch issue_dependency_read and issue_dependency_write to those. This removes the workaround, aligns the tools with the rest of the REST-based issue tooling, and simplifies tests. - Rewrite issue_dependencies.go on REST: page-based pagination for reads, and a single Issues.Get to resolve the blocking issue's database ID for writes. Preserve the tool surface, self-dependency guard and cross-repo support. - Rewrite the issue dependency tests on the REST mock helpers. - Regenerate the read toolsnap (cursor -> page pagination) and docs. Refs #950 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/feature-flags.md | 2 +- docs/insiders-features.md | 2 +- ...dependency_read_ff_issue_dependencies.snap | 9 +- pkg/github/issue_dependencies.go | 329 +++++++---------- pkg/github/issue_dependencies_test.go | 336 +++++++----------- 5 files changed, 282 insertions(+), 396 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 7b29698a93..32891af62e 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -310,7 +310,6 @@ runtime behavior (such as output formatting) won't appear here. - **issue_dependency_read** - Read issue dependencies - **Required OAuth Scopes**: `repo` - - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `issue_number`: The number of the issue (number, required) - `method`: The read operation to perform on a single issue's dependencies. Options are: @@ -318,6 +317,7 @@ runtime behavior (such as output formatting) won't appear here. 2. get_blocking - List the issues that this issue blocks. (string, required) - `owner`: The owner of the repository (string, required) + - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: The name of the repository (string, required) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index a7a3ceb5d3..10df187a91 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -105,7 +105,6 @@ The list below is generated from the Go source. It covers tool **inventory and s - **issue_dependency_read** - Read issue dependencies - **Required OAuth Scopes**: `repo` - - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `issue_number`: The number of the issue (number, required) - `method`: The read operation to perform on a single issue's dependencies. Options are: @@ -113,6 +112,7 @@ The list below is generated from the Go source. It covers tool **inventory and s 2. get_blocking - List the issues that this issue blocks. (string, required) - `owner`: The owner of the repository (string, required) + - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: The name of the repository (string, required) diff --git a/pkg/github/__toolsnaps__/issue_dependency_read_ff_issue_dependencies.snap b/pkg/github/__toolsnaps__/issue_dependency_read_ff_issue_dependencies.snap index 37b4e8f113..cf6c3662f1 100644 --- a/pkg/github/__toolsnaps__/issue_dependency_read_ff_issue_dependencies.snap +++ b/pkg/github/__toolsnaps__/issue_dependency_read_ff_issue_dependencies.snap @@ -7,10 +7,6 @@ "description": "Read an issue's dependency relationships in a GitHub repository: the issues that block it (blocked_by) or the issues it blocks (blocking).", "inputSchema": { "properties": { - "after": { - "description": "Cursor for pagination. Use the cursor from the previous response.", - "type": "string" - }, "issue_number": { "description": "The number of the issue", "type": "number" @@ -27,6 +23,11 @@ "description": "The owner of the repository", "type": "string" }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, "perPage": { "description": "Results per page for pagination (min 1, max 100)", "maximum": 100, diff --git a/pkg/github/issue_dependencies.go b/pkg/github/issue_dependencies.go index 6fbcc9eed7..6fd7ade0cf 100644 --- a/pkg/github/issue_dependencies.go +++ b/pkg/github/issue_dependencies.go @@ -3,6 +3,8 @@ package github import ( "context" "fmt" + "io" + "net/http" "strings" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -10,59 +12,11 @@ import ( "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v89/github" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/shurcooL/githubv4" ) -// AddBlockedByInput represents the input for the addBlockedBy GraphQL mutation. -// The pinned githubv4 library predates the dependency mutations, so the input -// type is declared here. The Go type name must match the GraphQL input type name. -type AddBlockedByInput struct { - IssueID githubv4.ID `json:"issueId"` - BlockingIssueID githubv4.ID `json:"blockingIssueId"` - ClientMutationID *githubv4.String `json:"clientMutationId,omitempty"` -} - -// RemoveBlockedByInput represents the input for the removeBlockedBy GraphQL mutation. -type RemoveBlockedByInput struct { - IssueID githubv4.ID `json:"issueId"` - BlockingIssueID githubv4.ID `json:"blockingIssueId"` - ClientMutationID *githubv4.String `json:"clientMutationId,omitempty"` -} - -// dependencyIssueNode is the minimal projection returned for each related issue -// in a blocked-by / blocking listing. -type dependencyIssueNode struct { - Number githubv4.Int - Title githubv4.String - State githubv4.String - URL githubv4.String - Repository struct { - NameWithOwner githubv4.String - } -} - -// dependencyConnection mirrors the shape of an IssueConnection returned by the -// blockedBy / blocking fields. -type dependencyConnection struct { - TotalCount githubv4.Int - PageInfo struct { - HasNextPage githubv4.Boolean - EndCursor githubv4.String - } - Nodes []dependencyIssueNode -} - -// minimalDependencyIssue is the JSON-serialised form of a related issue. -type minimalDependencyIssue struct { - Number int `json:"number"` - Title string `json:"title"` - State string `json:"state"` - URL string `json:"url"` - Repository string `json:"repository"` -} - // IssueDependencyRead creates a tool to read an issue's blocked-by and blocking // relationships. It is a separate, feature-flagged tool (rather than a method on // the default issue_read) so the whole dependency capability can be gated as a @@ -95,7 +49,7 @@ Options are: }, Required: []string{"method", "owner", "repo", "issue_number"}, } - WithCursorPagination(schema) + WithPagination(schema) st := NewTool( ToolsetMetadataIssues, @@ -127,29 +81,23 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } - if _, pageProvided := args["page"]; pageProvided { - return utils.NewToolResultError("This tool uses cursor-based pagination. Use the 'after' parameter with the 'endCursor' value from the previous response instead of 'page'."), nil, nil - } - pagination, err := OptionalCursorPaginationParams(args) - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - gqlPagination, err := pagination.ToGraphQLParams() + pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + opts := &github.ListOptions{Page: pagination.Page, PerPage: pagination.PerPage} - gqlClient, err := deps.GetGQLClient(ctx) + client, err := deps.GetClient(ctx) if err != nil { - return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } switch method { case "get_blocked_by": - result, err := GetIssueBlockedBy(ctx, gqlClient, owner, repo, issueNumber, gqlPagination) + result, err := GetIssueBlockedBy(ctx, client, owner, repo, issueNumber, opts) return result, nil, err case "get_blocking": - result, err := GetIssueBlocking(ctx, gqlClient, owner, repo, issueNumber, gqlPagination) + result, err := GetIssueBlocking(ctx, client, owner, repo, issueNumber, opts) return result, nil, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil @@ -159,79 +107,81 @@ Options are: return st } -func dependencyQueryVars(owner, repo string, issueNumber int, pagination *GraphQLPaginationParams) map[string]any { - vars := map[string]any{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "issueNumber": githubv4.Int(issueNumber), // #nosec G115 - issue numbers are always small positive integers - "first": githubv4.Int(*pagination.First), - } - if pagination.After != nil { - vars["after"] = githubv4.String(*pagination.After) - } else { - vars["after"] = (*githubv4.String)(nil) +// GetIssueBlockedBy lists the issues that block the given issue. +func GetIssueBlockedBy(ctx context.Context, client *github.Client, owner, repo string, issueNumber int, opts *github.ListOptions) (*mcp.CallToolResult, error) { + issues, resp, err := client.Issues.ListBlockedBy(ctx, owner, repo, int64(issueNumber), opts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list blocked-by issues", resp, err), nil } - return vars -} + defer func() { _ = resp.Body.Close() }() -func dependencyResult(conn dependencyConnection) *mcp.CallToolResult { - issues := make([]minimalDependencyIssue, 0, len(conn.Nodes)) - for _, node := range conn.Nodes { - issues = append(issues, minimalDependencyIssue{ - Number: int(node.Number), - Title: string(node.Title), - State: string(node.State), - URL: string(node.URL), - Repository: string(node.Repository.NameWithOwner), - }) + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list blocked-by issues", resp, body), nil } - return MarshalledTextResult(map[string]any{ - "issues": issues, - "totalCount": int(conn.TotalCount), - "pageInfo": map[string]any{ - "hasNextPage": bool(conn.PageInfo.HasNextPage), - "endCursor": string(conn.PageInfo.EndCursor), - }, - }) + return dependencyReadResult(issues, resp), nil } -// GetIssueBlockedBy lists the issues that block the given issue. -func GetIssueBlockedBy(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber int, pagination *GraphQLPaginationParams) (*mcp.CallToolResult, error) { - var query struct { - Repository struct { - Issue struct { - BlockedBy dependencyConnection `graphql:"blockedBy(first: $first, after: $after)"` - } `graphql:"issue(number: $issueNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` +// GetIssueBlocking lists the issues that the given issue blocks. +func GetIssueBlocking(ctx context.Context, client *github.Client, owner, repo string, issueNumber int, opts *github.ListOptions) (*mcp.CallToolResult, error) { + issues, resp, err := client.Issues.ListBlocking(ctx, owner, repo, int64(issueNumber), opts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list blocking issues", resp, err), nil } + defer func() { _ = resp.Body.Close() }() - if err := client.Query(ctx, &query, dependencyQueryVars(owner, repo, issueNumber, pagination)); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get blocked-by issues", err), nil + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to list blocking issues", resp, body), nil } - return dependencyResult(query.Repository.Issue.BlockedBy), nil + return dependencyReadResult(issues, resp), nil } -// GetIssueBlocking lists the issues that the given issue blocks. -func GetIssueBlocking(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber int, pagination *GraphQLPaginationParams) (*mcp.CallToolResult, error) { - var query struct { - Repository struct { - Issue struct { - Blocking dependencyConnection `graphql:"blocking(first: $first, after: $after)"` - } `graphql:"issue(number: $issueNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` +// dependencyReadResult projects a list of related issues into the minimal +// dependency shape and attaches page-based pagination info. +func dependencyReadResult(issues []*github.Issue, resp *github.Response) *mcp.CallToolResult { + refs := make([]MinimalIssueRef, 0, len(issues)) + for _, issue := range issues { + if issue == nil { + continue + } + refs = append(refs, issueToDependencyRef(issue)) } + return MarshalledTextResult(map[string]any{ + "issues": refs, + "pageInfo": map[string]any{ + "hasNextPage": resp.NextPage != 0, + "nextPage": resp.NextPage, + }, + }) +} - if err := client.Query(ctx, &query, dependencyQueryVars(owner, repo, issueNumber, pagination)); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get blocking issues", err), nil +// issueToDependencyRef converts a REST issue into the compact reference used by +// the dependency tools, deriving the "owner/repo" name from the issue's +// repository URL. +func issueToDependencyRef(issue *github.Issue) MinimalIssueRef { + ref := MinimalIssueRef{ + Number: issue.GetNumber(), + Title: issue.GetTitle(), + State: issue.GetState(), + URL: issue.GetHTMLURL(), + } + if owner, repo, ok := parseRepositoryURL(issue.GetRepositoryURL()); ok { + ref.Repository = owner + "/" + repo } - return dependencyResult(query.Repository.Issue.Blocking), nil + return ref } // IssueDependencyWrite creates a tool to add or remove an issue dependency -// (blocked-by / blocking) relationship. It accepts issue numbers and resolves -// them to GraphQL node IDs before calling the addBlockedBy / removeBlockedBy -// mutations. "blocking" is the inverse of "blocked_by", so both directions are -// served by the same mutation pair with the issue arguments swapped. +// (blocked-by / blocking) relationship. The REST dependency endpoints are always +// expressed as "the blocked issue is blocked_by the blocking issue", so both +// directions are served by the same endpoint pair with the two issues swapped. func IssueDependencyWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, @@ -347,102 +297,97 @@ Options are: return utils.NewToolResultError("an issue cannot block or depend on itself"), nil, nil } - gqlClient, err := deps.GetGQLClient(ctx) - if err != nil { - return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil + // Map the subject/related pair onto the blocked/blocking roles the REST + // endpoints expect. For type 'blocked_by' the subject is the blocked + // issue; for 'blocking' the subject blocks the related issue, so the + // roles swap. + blocked := issueCoordinate{owner: owner, repo: repo, number: issueNumber} + blocking := issueCoordinate{owner: relatedOwner, repo: relatedRepo, number: relatedIssueNumber} + if relationshipType == "blocking" { + blocked, blocking = blocking, blocked } - subjectID, relatedID, err := resolveIssueNodeIDs(ctx, gqlClient, owner, repo, issueNumber, relatedOwner, relatedRepo, relatedIssueNumber) + client, err := deps.GetClient(ctx) if err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to resolve issues", err), nil, nil - } - - // "A blocks B" is recorded as addBlockedBy(issueId: B, blockingIssueId: A). - // For type 'blocked_by' the subject is blocked by the related issue. - // For type 'blocking' the subject blocks the related issue, so the roles swap. - blockedID, blockingID := subjectID, relatedID - if relationshipType == "blocking" { - blockedID, blockingID = relatedID, subjectID + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } - result, err := writeBlockedByRelationship(ctx, gqlClient, method, blockedID, blockingID) + result, err := writeIssueDependency(ctx, client, method, blocked, blocking) return result, nil, err }) st.FeatureFlagEnable = FeatureFlagIssueDependencies return st } -// resolveIssueNodeIDs resolves the subject and related issue numbers to their -// GraphQL node IDs in a single aliased query. -func resolveIssueNodeIDs(ctx context.Context, client *githubv4.Client, owner, repo string, issueNumber int, relatedOwner, relatedRepo string, relatedIssueNumber int) (githubv4.ID, githubv4.ID, error) { - var query struct { - Subject struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $subjectNumber)"` - } `graphql:"subject: repository(owner: $subjectOwner, name: $subjectRepo)"` - Related struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $relatedNumber)"` - } `graphql:"related: repository(owner: $relatedOwner, name: $relatedRepo)"` - } - vars := map[string]any{ - "subjectOwner": githubv4.String(owner), - "subjectRepo": githubv4.String(repo), - "subjectNumber": githubv4.Int(issueNumber), // #nosec G115 - issue numbers are always small positive integers - "relatedOwner": githubv4.String(relatedOwner), - "relatedRepo": githubv4.String(relatedRepo), - "relatedNumber": githubv4.Int(relatedIssueNumber), // #nosec G115 - issue numbers are always small positive integers - } - if err := client.Query(ctx, &query, vars); err != nil { - return "", "", err - } - return query.Subject.Issue.ID, query.Related.Issue.ID, nil +// issueCoordinate identifies an issue by repository and number. +type issueCoordinate struct { + owner string + repo string + number int } -// writeBlockedByRelationship runs the addBlockedBy / removeBlockedBy mutation and -// returns a minimal description of the affected issues. -func writeBlockedByRelationship(ctx context.Context, client *githubv4.Client, method string, blockedID, blockingID githubv4.ID) (*mcp.CallToolResult, error) { - type mutationIssue struct { - Number githubv4.Int - URL githubv4.String +// writeIssueDependency resolves the blocking issue to its global database ID and +// then adds or removes the blocked-by relationship on the blocked issue. +func writeIssueDependency(ctx context.Context, client *github.Client, method string, blocked, blocking issueCoordinate) (*mcp.CallToolResult, error) { + // The REST API identifies the blocking issue by its global database ID + // (not its number), so resolve the number to an ID first. + blockingIssue, resp, err := client.Issues.Get(ctx, blocking.owner, blocking.repo, blocking.number) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to resolve blocking issue", resp, err), nil } + _ = resp.Body.Close() + blockingID := blockingIssue.GetID() switch method { case "add": - var mutation struct { - AddBlockedBy struct { - Issue mutationIssue - BlockingIssue mutationIssue - } `graphql:"addBlockedBy(input: $input)"` + blockedIssue, opResp, err := client.Issues.AddBlockedBy(ctx, blocked.owner, blocked.repo, int64(blocked.number), github.IssueDependencyRequest{IssueID: blockingID}) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to add issue dependency", opResp, err), nil } - input := AddBlockedByInput{IssueID: blockedID, BlockingIssueID: blockingID} - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to add issue dependency", err), nil + defer func() { _ = opResp.Body.Close() }() + if opResp.StatusCode != http.StatusCreated { + body, readErr := io.ReadAll(opResp.Body) + if readErr != nil { + return nil, fmt.Errorf("failed to read response body: %w", readErr) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add issue dependency", opResp, body), nil } - return MarshalledTextResult(map[string]any{ - "message": "dependency added", - "blocked_issue": map[string]any{"number": int(mutation.AddBlockedBy.Issue.Number), "url": string(mutation.AddBlockedBy.Issue.URL)}, - "blocking_issue": map[string]any{"number": int(mutation.AddBlockedBy.BlockingIssue.Number), "url": string(mutation.AddBlockedBy.BlockingIssue.URL)}, - }), nil + return dependencyWriteResult("dependency added", blockedIssue, blockingIssue, blocked, blocking), nil case "remove": - var mutation struct { - RemoveBlockedBy struct { - Issue mutationIssue - BlockingIssue mutationIssue - } `graphql:"removeBlockedBy(input: $input)"` + blockedIssue, opResp, err := client.Issues.RemoveBlockedBy(ctx, blocked.owner, blocked.repo, int64(blocked.number), blockingID) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to remove issue dependency", opResp, err), nil } - input := RemoveBlockedByInput{IssueID: blockedID, BlockingIssueID: blockingID} - if err := client.Mutate(ctx, &mutation, input, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to remove issue dependency", err), nil + defer func() { _ = opResp.Body.Close() }() + if opResp.StatusCode != http.StatusOK { + body, readErr := io.ReadAll(opResp.Body) + if readErr != nil { + return nil, fmt.Errorf("failed to read response body: %w", readErr) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to remove issue dependency", opResp, body), nil } - return MarshalledTextResult(map[string]any{ - "message": "dependency removed", - "blocked_issue": map[string]any{"number": int(mutation.RemoveBlockedBy.Issue.Number), "url": string(mutation.RemoveBlockedBy.Issue.URL)}, - "blocking_issue": map[string]any{"number": int(mutation.RemoveBlockedBy.BlockingIssue.Number), "url": string(mutation.RemoveBlockedBy.BlockingIssue.URL)}, - }), nil + return dependencyWriteResult("dependency removed", blockedIssue, blockingIssue, blocked, blocking), nil default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil } } + +// dependencyWriteResult builds the minimal description of the affected issues. +// The blocked issue comes from the mutation response and the blocking issue from +// the earlier resolve; each falls back to its known coordinate when the API +// response omits the repository URL. +func dependencyWriteResult(message string, blockedIssue, blockingIssue *github.Issue, blocked, blocking issueCoordinate) *mcp.CallToolResult { + blockedRef := issueToDependencyRef(blockedIssue) + if blockedRef.Repository == "" { + blockedRef.Repository = blocked.owner + "/" + blocked.repo + } + blockingRef := issueToDependencyRef(blockingIssue) + if blockingRef.Repository == "" { + blockingRef.Repository = blocking.owner + "/" + blocking.repo + } + return MarshalledTextResult(map[string]any{ + "message": message, + "blocked_issue": blockedRef, + "blocking_issue": blockingRef, + }) +} diff --git a/pkg/github/issue_dependencies_test.go b/pkg/github/issue_dependencies_test.go index 47859e9378..4a8fcf9779 100644 --- a/pkg/github/issue_dependencies_test.go +++ b/pkg/github/issue_dependencies_test.go @@ -3,17 +3,33 @@ package github import ( "context" "encoding/json" + "net/http" + "strconv" "testing" - "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/jsonschema-go/jsonschema" - "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +const ( + endpointBlockedBy = EndpointPattern("GET /repos/{owner}/{repo}/issues/{issue_number}/dependencies/blocked_by") + endpointBlocking = EndpointPattern("GET /repos/{owner}/{repo}/issues/{issue_number}/dependencies/blocking") + endpointAddBlock = EndpointPattern("POST /repos/{owner}/{repo}/issues/{issue_number}/dependencies/blocked_by") + endpointRemoveBlk = EndpointPattern("DELETE /repos/{owner}/{repo}/issues/{issue_number}/dependencies/blocked_by/{issue_id}") + endpointGetIssue = EndpointPattern("GET /repos/{owner}/{repo}/issues/{issue_number}") +) + +// jsonHandler writes the given status code and JSON-encoded body. +func jsonHandler(status int, body any) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + _, _ = w.Write(MustMarshal(body)) + } +} + func Test_IssueDependencyRead(t *testing.T) { // Verify tool definition once (flag-gated variant snap) serverTool := IssueDependencyRead(translations.NullTranslationHelper) @@ -29,98 +45,47 @@ func Test_IssueDependencyRead(t *testing.T) { assert.Contains(t, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") assert.Contains(t, schema.Properties, "issue_number") + assert.Contains(t, schema.Properties, "page") + assert.Contains(t, schema.Properties, "perPage") assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "issue_number"}) - blockedByQuery := githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Issue struct { - BlockedBy dependencyConnection `graphql:"blockedBy(first: $first, after: $after)"` - } `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), - "first": githubv4.Int(30), - "after": (*githubv4.String)(nil), + blockedByIssues := []map[string]any{ + { + "number": 7, + "title": "Blocker", + "state": "open", + "html_url": "https://github.com/owner/repo/issues/7", + "repository_url": "https://api.github.com/repos/owner/repo", }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "blockedBy": map[string]any{ - "totalCount": 1, - "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", - }, - "nodes": []map[string]any{ - { - "number": 7, - "title": "Blocker", - "state": "OPEN", - "url": "https://github.com/owner/repo/issues/7", - "repository": map[string]any{"nameWithOwner": "owner/repo"}, - }, - }, - }, - }, - }, - }), - ) - - blockingQuery := githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Issue struct { - Blocking dependencyConnection `graphql:"blocking(first: $first, after: $after)"` - } `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), - "first": githubv4.Int(30), - "after": (*githubv4.String)(nil), + } + blockingIssues := []map[string]any{ + { + "number": 8, + "title": "Blocked A", + "state": "open", + "html_url": "https://github.com/owner/repo/issues/8", + "repository_url": "https://api.github.com/repos/owner/repo", }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "blocking": map[string]any{ - "totalCount": 2, - "pageInfo": map[string]any{ - "hasNextPage": true, - "endCursor": "Y3Vyc29y", - }, - "nodes": []map[string]any{ - { - "number": 8, - "title": "Blocked A", - "state": "OPEN", - "url": "https://github.com/owner/repo/issues/8", - "repository": map[string]any{"nameWithOwner": "owner/repo"}, - }, - { - "number": 9, - "title": "Blocked B", - "state": "CLOSED", - "url": "https://github.com/owner/repo/issues/9", - "repository": map[string]any{"nameWithOwner": "owner/repo"}, - }, - }, - }, - }, - }, - }), - ) + { + "number": 9, + "title": "Blocked B", + "state": "closed", + "html_url": "https://github.com/owner/repo/issues/9", + "repository_url": "https://api.github.com/repos/owner/repo", + }, + } + + // A handler that also advertises a next page via the Link header. + blockingHandler := func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Link", `; rel="next"`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(MustMarshal(blockingIssues)) + } tests := []struct { name string method string - matcher githubv4mock.Matcher - expectError bool + option MockBackendOption expectedCount int expectedFirst int expectedNext bool @@ -128,7 +93,7 @@ func Test_IssueDependencyRead(t *testing.T) { { name: "get_blocked_by returns blockers", method: "get_blocked_by", - matcher: blockedByQuery, + option: WithRequestMatch(endpointBlockedBy, blockedByIssues), expectedCount: 1, expectedFirst: 7, expectedNext: false, @@ -136,7 +101,7 @@ func Test_IssueDependencyRead(t *testing.T) { { name: "get_blocking returns blocked issues", method: "get_blocking", - matcher: blockingQuery, + option: WithRequestMatchHandler(endpointBlocking, blockingHandler), expectedCount: 2, expectedFirst: 8, expectedNext: true, @@ -145,8 +110,8 @@ func Test_IssueDependencyRead(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(tc.matcher)) - deps := BaseDeps{GQLClient: gqlClient} + client := mustNewGHClient(t, NewMockedHTTPClient(tc.option)) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ @@ -161,17 +126,17 @@ func Test_IssueDependencyRead(t *testing.T) { text := getTextResult(t, result) var payload struct { - Issues []minimalDependencyIssue `json:"issues"` - Total int `json:"totalCount"` - Page struct { - HasNextPage bool `json:"hasNextPage"` - EndCursor string `json:"endCursor"` + Issues []MinimalIssueRef `json:"issues"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + NextPage int `json:"nextPage"` } `json:"pageInfo"` } require.NoError(t, json.Unmarshal([]byte(text.Text), &payload)) require.Len(t, payload.Issues, tc.expectedCount) assert.Equal(t, tc.expectedFirst, payload.Issues[0].Number) - assert.Equal(t, tc.expectedNext, payload.Page.HasNextPage) + assert.Equal(t, "owner/repo", payload.Issues[0].Repository) + assert.Equal(t, tc.expectedNext, payload.PageInfo.HasNextPage) }) } } @@ -179,31 +144,34 @@ func Test_IssueDependencyRead(t *testing.T) { func Test_IssueDependencyRead_Errors(t *testing.T) { serverTool := IssueDependencyRead(translations.NullTranslationHelper) - t.Run("rejects page-based pagination", func(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient()) - deps := BaseDeps{GQLClient: gqlClient} + t.Run("missing required param", func(t *testing.T) { + client := mustNewGHClient(t, NewMockedHTTPClient()) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ - "method": "get_blocked_by", - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "page": float64(2), + "method": "get_blocked_by", + "owner": "owner", + "repo": "repo", }) result, err := handler(ContextWithDeps(context.Background(), deps), &request) require.NoError(t, err) - errText := getErrorResult(t, result) - assert.Contains(t, errText.Text, "cursor-based pagination") + getErrorResult(t, result) }) - t.Run("missing required param", func(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient()) - deps := BaseDeps{GQLClient: gqlClient} + t.Run("API error is surfaced", func(t *testing.T) { + client := mustNewGHClient(t, NewMockedHTTPClient( + WithRequestMatchHandler(endpointBlockedBy, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + })), + )) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ - "method": "get_blocked_by", - "owner": "owner", - "repo": "repo", + "method": "get_blocked_by", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), }) result, err := handler(ContextWithDeps(context.Background(), deps), &request) require.NoError(t, err) @@ -228,114 +196,82 @@ func Test_IssueDependencyWrite(t *testing.T) { assert.Contains(t, schema.Properties, "related_issue_number") assert.ElementsMatch(t, schema.Required, []string{"method", "type", "owner", "repo", "issue_number", "related_issue_number"}) - resolveMatcher := func(subjectID, relatedID string) githubv4mock.Matcher { - return githubv4mock.NewQueryMatcher( - struct { - Subject struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $subjectNumber)"` - } `graphql:"subject: repository(owner: $subjectOwner, name: $subjectRepo)"` - Related struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $relatedNumber)"` - } `graphql:"related: repository(owner: $relatedOwner, name: $relatedRepo)"` - }{}, - map[string]any{ - "subjectOwner": githubv4.String("owner"), - "subjectRepo": githubv4.String("repo"), - "subjectNumber": githubv4.Int(1), - "relatedOwner": githubv4.String("owner"), - "relatedRepo": githubv4.String("repo"), - "relatedNumber": githubv4.Int(2), - }, - githubv4mock.DataResponse(map[string]any{ - "subject": map[string]any{"issue": map[string]any{"id": subjectID}}, - "related": map[string]any{"issue": map[string]any{"id": relatedID}}, - }), - ) - } - - type mutationIssue struct { - Number githubv4.Int - URL githubv4.String - } - - addMutation := func(issueID, blockingID string) githubv4mock.Matcher { - return githubv4mock.NewMutationMatcher( - struct { - AddBlockedBy struct { - Issue mutationIssue - BlockingIssue mutationIssue - } `graphql:"addBlockedBy(input: $input)"` - }{}, - AddBlockedByInput{IssueID: githubv4.ID(issueID), BlockingIssueID: githubv4.ID(blockingID)}, - nil, - githubv4mock.DataResponse(map[string]any{ - "addBlockedBy": map[string]any{ - "issue": map[string]any{"number": 1, "url": "https://github.com/owner/repo/issues/1"}, - "blockingIssue": map[string]any{"number": 2, "url": "https://github.com/owner/repo/issues/2"}, - }, - }), - ) + // issue returned by the blocking-issue resolve GET; its id is what the + // dependency endpoints operate on. + resolvedIssue := func(number, id int) map[string]any { + return map[string]any{ + "id": id, + "number": number, + "title": "Resolved", + "state": "open", + "html_url": "https://github.com/owner/repo/issues/" + strconv.Itoa(number), + "repository_url": "https://api.github.com/repos/owner/repo", + } } - - removeMutation := func(issueID, blockingID string) githubv4mock.Matcher { - return githubv4mock.NewMutationMatcher( - struct { - RemoveBlockedBy struct { - Issue mutationIssue - BlockingIssue mutationIssue - } `graphql:"removeBlockedBy(input: $input)"` - }{}, - RemoveBlockedByInput{IssueID: githubv4.ID(issueID), BlockingIssueID: githubv4.ID(blockingID)}, - nil, - githubv4mock.DataResponse(map[string]any{ - "removeBlockedBy": map[string]any{ - "issue": map[string]any{"number": 1, "url": "https://github.com/owner/repo/issues/1"}, - "blockingIssue": map[string]any{"number": 2, "url": "https://github.com/owner/repo/issues/2"}, - }, - }), - ) + // issue returned by the add/remove endpoints (the blocked issue). + blockedIssue := func(number int) map[string]any { + return map[string]any{ + "number": number, + "title": "Blocked", + "state": "open", + "html_url": "https://github.com/owner/repo/issues/" + strconv.Itoa(number), + "repository_url": "https://api.github.com/repos/owner/repo", + } } tests := []struct { name string method string relationship string - matchers []githubv4mock.Matcher + options []MockBackendOption expectedMessage string + expectedBlocked int + expectedBlockng int }{ { name: "add blocked_by uses subject as blocked", method: "add", relationship: "blocked_by", - // subject(1) is blocked by related(2): issueId=subject, blockingIssueId=related - matchers: []githubv4mock.Matcher{resolveMatcher("I_subject", "I_related"), addMutation("I_subject", "I_related")}, + // subject(1) is blocked by related(2): resolve related(2), block issue 1. + options: []MockBackendOption{ + WithRequestMatch(endpointGetIssue, resolvedIssue(2, 1002)), + WithRequestMatchHandler(endpointAddBlock, jsonHandler(http.StatusCreated, blockedIssue(1))), + }, expectedMessage: "dependency added", + expectedBlocked: 1, + expectedBlockng: 2, }, { name: "add blocking swaps roles", method: "add", relationship: "blocking", - // subject(1) blocks related(2): issueId=related, blockingIssueId=subject - matchers: []githubv4mock.Matcher{resolveMatcher("I_subject", "I_related"), addMutation("I_related", "I_subject")}, + // subject(1) blocks related(2): resolve subject(1), block issue 2. + options: []MockBackendOption{ + WithRequestMatch(endpointGetIssue, resolvedIssue(1, 1001)), + WithRequestMatchHandler(endpointAddBlock, jsonHandler(http.StatusCreated, blockedIssue(2))), + }, expectedMessage: "dependency added", + expectedBlocked: 2, + expectedBlockng: 1, }, { - name: "remove blocked_by", - method: "remove", - relationship: "blocked_by", - matchers: []githubv4mock.Matcher{resolveMatcher("I_subject", "I_related"), removeMutation("I_subject", "I_related")}, + name: "remove blocked_by", + method: "remove", + relationship: "blocked_by", + options: []MockBackendOption{ + WithRequestMatch(endpointGetIssue, resolvedIssue(2, 1002)), + WithRequestMatch(endpointRemoveBlk, blockedIssue(1)), + }, expectedMessage: "dependency removed", + expectedBlocked: 1, + expectedBlockng: 2, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(tc.matchers...)) - deps := BaseDeps{GQLClient: gqlClient} + client := mustNewGHClient(t, NewMockedHTTPClient(tc.options...)) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ @@ -352,17 +288,21 @@ func Test_IssueDependencyWrite(t *testing.T) { text := getTextResult(t, result) var payload struct { - Message string `json:"message"` + Message string `json:"message"` + BlockedIssue MinimalIssueRef `json:"blocked_issue"` + BlockingIssue MinimalIssueRef `json:"blocking_issue"` } require.NoError(t, json.Unmarshal([]byte(text.Text), &payload)) assert.Equal(t, tc.expectedMessage, payload.Message) + assert.Equal(t, tc.expectedBlocked, payload.BlockedIssue.Number) + assert.Equal(t, tc.expectedBlockng, payload.BlockingIssue.Number) }) } t.Run("self dependency fails before any API call", func(t *testing.T) { - // Register no matchers: the handler must return before resolving node IDs or mutating. - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient()) - deps := BaseDeps{GQLClient: gqlClient} + // Register no handlers: the handler must return before resolving or mutating. + client := mustNewGHClient(t, NewMockedHTTPClient()) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{ @@ -414,8 +354,8 @@ func Test_IssueDependencyWrite_Validation(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient()) - deps := BaseDeps{GQLClient: gqlClient} + client := mustNewGHClient(t, NewMockedHTTPClient()) + deps := BaseDeps{Client: client} handler := serverTool.Handler(deps) request := createMCPRequest(tc.args) result, err := handler(ContextWithDeps(context.Background(), deps), &request) From e556a6e137943dfa1e51b9c81d68d3245f8495f7 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Wed, 8 Jul 2026 15:11:12 +0100 Subject: [PATCH 2/2] Normalize issue dependency ref state to match other tools The REST issue-dependency endpoints return lower-case issue states (open/closed), whereas the previous GraphQL implementation and the sibling get_parent tool populate MinimalIssueRef.State from the GraphQL IssueState enum (OPEN/CLOSED). Upper-case the state in issueToDependencyRef so the field stays consistent across every tool that emits a MinimalIssueRef, and guard against a nil issue while here. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/github/issue_dependencies.go | 9 +++++++-- pkg/github/issue_dependencies_test.go | 6 ++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/github/issue_dependencies.go b/pkg/github/issue_dependencies.go index 6fd7ade0cf..49533232d5 100644 --- a/pkg/github/issue_dependencies.go +++ b/pkg/github/issue_dependencies.go @@ -164,12 +164,17 @@ func dependencyReadResult(issues []*github.Issue, resp *github.Response) *mcp.Ca // issueToDependencyRef converts a REST issue into the compact reference used by // the dependency tools, deriving the "owner/repo" name from the issue's -// repository URL. +// repository URL. The state is upper-cased so it matches the GraphQL-sourced +// state (e.g. "OPEN"/"CLOSED") that MinimalIssueRef carries for the other issue +// tools such as get_parent, keeping the field consistent across tools. func issueToDependencyRef(issue *github.Issue) MinimalIssueRef { + if issue == nil { + return MinimalIssueRef{} + } ref := MinimalIssueRef{ Number: issue.GetNumber(), Title: issue.GetTitle(), - State: issue.GetState(), + State: strings.ToUpper(issue.GetState()), URL: issue.GetHTMLURL(), } if owner, repo, ok := parseRepositoryURL(issue.GetRepositoryURL()); ok { diff --git a/pkg/github/issue_dependencies_test.go b/pkg/github/issue_dependencies_test.go index 4a8fcf9779..6af9c504ed 100644 --- a/pkg/github/issue_dependencies_test.go +++ b/pkg/github/issue_dependencies_test.go @@ -88,6 +88,7 @@ func Test_IssueDependencyRead(t *testing.T) { option MockBackendOption expectedCount int expectedFirst int + expectedState string expectedNext bool }{ { @@ -96,6 +97,7 @@ func Test_IssueDependencyRead(t *testing.T) { option: WithRequestMatch(endpointBlockedBy, blockedByIssues), expectedCount: 1, expectedFirst: 7, + expectedState: "OPEN", expectedNext: false, }, { @@ -104,6 +106,7 @@ func Test_IssueDependencyRead(t *testing.T) { option: WithRequestMatchHandler(endpointBlocking, blockingHandler), expectedCount: 2, expectedFirst: 8, + expectedState: "OPEN", expectedNext: true, }, } @@ -136,6 +139,9 @@ func Test_IssueDependencyRead(t *testing.T) { require.Len(t, payload.Issues, tc.expectedCount) assert.Equal(t, tc.expectedFirst, payload.Issues[0].Number) assert.Equal(t, "owner/repo", payload.Issues[0].Repository) + // State is normalized to upper case to match the GraphQL-sourced + // state used by other MinimalIssueRef producers (e.g. get_parent). + assert.Equal(t, tc.expectedState, payload.Issues[0].State) assert.Equal(t, tc.expectedNext, payload.PageInfo.HasNextPage) }) }