From e2c6fe685ae2065333693eb88784675c652aea2f Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 4 Feb 2026 20:36:44 +0000 Subject: [PATCH 1/2] fix(security): remediate SonarQube vulnerabilities (S1192, S3776) - Define constants for duplicated string literals: - errFailedToGetPullRequest - errFailedToGetCurrentUser - errFailedToGetLatestReviewForCurrUser - Refactor GetPullRequestFiles to reduce cognitive complexity: - Extract parameter extraction into extractPullRequestFilesParams - Extract API call into fetchPullRequestFiles helper - Use MarshalledTextResult for response marshaling Addresses SonarQube issues: - S1192: Duplicated string literals (3 issues) - S3776: Cognitive complexity too high (1 issue) Co-Authored-By: parker.duff@codeium.com --- pkg/github/pullrequests.go | 105 ++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 36 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index bad822b13..e280c9e85 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -17,6 +17,59 @@ import ( "github.com/github/github-mcp-server/pkg/translations" ) +const ( + errFailedToGetPullRequest = "failed to get pull request" + errFailedToGetCurrentUser = "failed to get current user" + errFailedToGetLatestReviewForCurrUser = "failed to get latest review for current user" +) + +// pullRequestFilesParams holds the extracted parameters for GetPullRequestFiles +type pullRequestFilesParams struct { + owner string + repo string + pullNumber int + pagination PaginationParams +} + +// extractPullRequestFilesParams extracts and validates parameters for GetPullRequestFiles +func extractPullRequestFilesParams(request mcp.CallToolRequest) (*pullRequestFilesParams, error) { + owner, err := RequiredParam[string](request, "owner") + if err != nil { + return nil, err + } + repo, err := RequiredParam[string](request, "repo") + if err != nil { + return nil, err + } + pullNumber, err := RequiredInt(request, "pullNumber") + if err != nil { + return nil, err + } + pagination, err := OptionalPaginationParams(request) + if err != nil { + return nil, err + } + return &pullRequestFilesParams{ + owner: owner, + repo: repo, + pullNumber: pullNumber, + pagination: pagination, + }, nil +} + +// fetchPullRequestFiles fetches the files changed in a pull request +func fetchPullRequestFiles( + ctx context.Context, + client *github.Client, + params *pullRequestFilesParams, +) ([]*github.CommitFile, *github.Response, error) { + opts := &github.ListOptions{ + PerPage: params.pagination.perPage, + Page: params.pagination.page, + } + return client.PullRequests.ListFiles(ctx, params.owner, params.repo, params.pullNumber, opts) +} + // GetPullRequest creates a tool to get details of a specific pull request. func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("get_pull_request", @@ -59,7 +112,7 @@ func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to get pull request", + errFailedToGetPullRequest, resp, err, ), nil @@ -71,7 +124,7 @@ func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) if err != nil { return nil, fmt.Errorf("failed to read response body: %w", err) } - return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %s", string(body))), nil + return mcp.NewToolResultError(fmt.Sprintf("%s: %s", errFailedToGetPullRequest, string(body))), nil } r, err := json.Marshal(pr) @@ -601,19 +654,7 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper WithPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - owner, err := RequiredParam[string](request, "owner") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - repo, err := RequiredParam[string](request, "repo") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - pullNumber, err := RequiredInt(request, "pullNumber") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - pagination, err := OptionalPaginationParams(request) + params, err := extractPullRequestFilesParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -622,11 +663,8 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) } - opts := &github.ListOptions{ - PerPage: pagination.perPage, - Page: pagination.page, - } - files, resp, err := client.PullRequests.ListFiles(ctx, owner, repo, pullNumber, opts) + + files, resp, err := fetchPullRequestFiles(ctx, client, params) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request files", @@ -644,12 +682,7 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request files: %s", string(body))), nil } - r, err := json.Marshal(files) - if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) - } - - return mcp.NewToolResultText(string(r)), nil + return MarshalledTextResult(files), nil } } @@ -695,7 +728,7 @@ func GetPullRequestStatus(getClient GetClientFn, t translations.TranslationHelpe pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to get pull request", + errFailedToGetPullRequest, resp, err, ), nil @@ -707,7 +740,7 @@ func GetPullRequestStatus(getClient GetClientFn, t translations.TranslationHelpe if err != nil { return nil, fmt.Errorf("failed to read response body: %w", err) } - return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %s", string(body))), nil + return mcp.NewToolResultError(fmt.Sprintf("%s: %s", errFailedToGetPullRequest, string(body))), nil } // Get combined status for the head SHA @@ -1025,7 +1058,7 @@ func CreateAndSubmitPullRequestReview(getGQLClient GetGQLClientFn, t translation "prNum": githubv4.Int(params.PullNumber), }); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get pull request", + errFailedToGetPullRequest, err, ), nil } @@ -1119,7 +1152,7 @@ func CreatePendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. "prNum": githubv4.Int(params.PullNumber), }); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get pull request", + errFailedToGetPullRequest, err, ), nil } @@ -1240,7 +1273,7 @@ func AddPullRequestReviewCommentToPendingReview(getGQLClient GetGQLClientFn, t t if err := client.Query(ctx, &getViewerQuery, nil); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", + errFailedToGetCurrentUser, err, ), nil } @@ -1268,7 +1301,7 @@ func AddPullRequestReviewCommentToPendingReview(getGQLClient GetGQLClientFn, t t if err := client.Query(context.Background(), &getLatestReviewForViewerQuery, vars); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", + errFailedToGetLatestReviewForCurrUser, err, ), nil } @@ -1377,7 +1410,7 @@ func SubmitPendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. if err := client.Query(ctx, &getViewerQuery, nil); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", + errFailedToGetCurrentUser, err, ), nil } @@ -1405,7 +1438,7 @@ func SubmitPendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. if err := client.Query(context.Background(), &getLatestReviewForViewerQuery, vars); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", + errFailedToGetLatestReviewForCurrUser, err, ), nil } @@ -1501,7 +1534,7 @@ func DeletePendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. if err := client.Query(ctx, &getViewerQuery, nil); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", + errFailedToGetCurrentUser, err, ), nil } @@ -1529,7 +1562,7 @@ func DeletePendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. if err := client.Query(context.Background(), &getLatestReviewForViewerQuery, vars); err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", + errFailedToGetLatestReviewForCurrUser, err, ), nil } From 19d8d86680ead09a9bd58b2228fd1e32419daa0f Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 4 Feb 2026 20:40:40 +0000 Subject: [PATCH 2/2] fix: remove helper functions to address SonarCloud duplication warning - Reverted GetPullRequestFiles to inline parameter extraction - Kept constants for S1192 fix (duplicated string literals) - Kept MarshalledTextResult usage for cleaner marshaling - Removed pullRequestFilesParams struct and helper functions Co-Authored-By: parker.duff@codeium.com --- pkg/github/pullrequests.go | 68 ++++++++++---------------------------- 1 file changed, 18 insertions(+), 50 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index e280c9e85..2eff2193e 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -23,53 +23,6 @@ const ( errFailedToGetLatestReviewForCurrUser = "failed to get latest review for current user" ) -// pullRequestFilesParams holds the extracted parameters for GetPullRequestFiles -type pullRequestFilesParams struct { - owner string - repo string - pullNumber int - pagination PaginationParams -} - -// extractPullRequestFilesParams extracts and validates parameters for GetPullRequestFiles -func extractPullRequestFilesParams(request mcp.CallToolRequest) (*pullRequestFilesParams, error) { - owner, err := RequiredParam[string](request, "owner") - if err != nil { - return nil, err - } - repo, err := RequiredParam[string](request, "repo") - if err != nil { - return nil, err - } - pullNumber, err := RequiredInt(request, "pullNumber") - if err != nil { - return nil, err - } - pagination, err := OptionalPaginationParams(request) - if err != nil { - return nil, err - } - return &pullRequestFilesParams{ - owner: owner, - repo: repo, - pullNumber: pullNumber, - pagination: pagination, - }, nil -} - -// fetchPullRequestFiles fetches the files changed in a pull request -func fetchPullRequestFiles( - ctx context.Context, - client *github.Client, - params *pullRequestFilesParams, -) ([]*github.CommitFile, *github.Response, error) { - opts := &github.ListOptions{ - PerPage: params.pagination.perPage, - Page: params.pagination.page, - } - return client.PullRequests.ListFiles(ctx, params.owner, params.repo, params.pullNumber, opts) -} - // GetPullRequest creates a tool to get details of a specific pull request. func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("get_pull_request", @@ -654,7 +607,19 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper WithPagination(), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - params, err := extractPullRequestFilesParams(request) + owner, err := RequiredParam[string](request, "owner") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + repo, err := RequiredParam[string](request, "repo") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + pullNumber, err := RequiredInt(request, "pullNumber") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + pagination, err := OptionalPaginationParams(request) if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ -663,8 +628,11 @@ func GetPullRequestFiles(getClient GetClientFn, t translations.TranslationHelper if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) } - - files, resp, err := fetchPullRequestFiles(ctx, client, params) + opts := &github.ListOptions{ + PerPage: pagination.perPage, + Page: pagination.page, + } + files, resp, err := client.PullRequests.ListFiles(ctx, owner, repo, pullNumber, opts) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request files",