From f9a9740fa80e928397bd97105f744db36671013d Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 14 Oct 2025 18:24:28 +0000 Subject: [PATCH] Fix MBA-471: Resolve High severity string duplication and cognitive complexity issues - Extract error message constants to pkg/errors/constants.go - ErrContextMissingGitHubCtxErrors for context error messages - ErrFailedToGetGitHubClient for client initialization errors - ErrMissingRequiredParameter for parameter validation errors - Update pkg/errors/error.go to use ErrContextMissingGitHubCtxErrors constant at lines 75, 83, 99, 107 (changed from fmt.Errorf to errors.New to fix staticcheck SA1006) - Update pkg/github/server.go to use ErrMissingRequiredParameter constant at lines 74, 84 - Refactor get_file_contents handler in pkg/github/repositories.go to reduce cognitive complexity by extracting helper functions: - tryRawContentFetch: handles raw content API call - buildResourceURI: constructs resource URI based on sha/ref - createResourceContent: creates appropriate resource content type This reduces nesting levels from 5+ to 2-3, improving code maintainability. Addresses SonarQube High severity violations: - go:S1192 - String literals should not be duplicated - go:S3776 - Cognitive Complexity of functions should not be too high All tests pass and lint is clean. Co-Authored-By: Jia Wu --- pkg/errors/constants.go | 7 +++ pkg/errors/error.go | 9 ++-- pkg/github/repositories.go | 87 +++++++++++++++++++++----------------- pkg/github/server.go | 5 ++- 4 files changed, 64 insertions(+), 44 deletions(-) create mode 100644 pkg/errors/constants.go diff --git a/pkg/errors/constants.go b/pkg/errors/constants.go new file mode 100644 index 000000000..ced41c7d3 --- /dev/null +++ b/pkg/errors/constants.go @@ -0,0 +1,7 @@ +package errors + +const ( + ErrContextMissingGitHubCtxErrors = "context does not contain GitHubCtxErrors" + ErrFailedToGetGitHubClient = "failed to get GitHub client" + ErrMissingRequiredParameter = "missing required parameter: %s" +) diff --git a/pkg/errors/error.go b/pkg/errors/error.go index 9d81e9010..dde2cdaf6 100644 --- a/pkg/errors/error.go +++ b/pkg/errors/error.go @@ -2,6 +2,7 @@ package errors import ( "context" + "errors" "fmt" "github.com/google/go-github/v72/github" @@ -71,7 +72,7 @@ func GetGitHubAPIErrors(ctx context.Context) ([]*GitHubAPIError, error) { if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { return val.api, nil // return the slice of API errors from the context } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") + return nil, errors.New(ErrContextMissingGitHubCtxErrors) } // GetGitHubGraphQLErrors retrieves the slice of GitHubGraphQLErrors from the context. @@ -79,7 +80,7 @@ func GetGitHubGraphQLErrors(ctx context.Context) ([]*GitHubGraphQLError, error) if val, ok := ctx.Value(GitHubErrorKey{}).(*GitHubCtxErrors); ok { return val.graphQL, nil // return the slice of GraphQL errors from the context } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") + return nil, errors.New(ErrContextMissingGitHubCtxErrors) } func NewGitHubAPIErrorToCtx(ctx context.Context, message string, resp *github.Response, err error) (context.Context, error) { @@ -95,7 +96,7 @@ func addGitHubAPIErrorToContext(ctx context.Context, err *GitHubAPIError) (conte val.api = append(val.api, err) // append the error to the existing slice in the context return ctx, nil } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") + return nil, errors.New(ErrContextMissingGitHubCtxErrors) } func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError) (context.Context, error) { @@ -103,7 +104,7 @@ func addGitHubGraphQLErrorToContext(ctx context.Context, err *GitHubGraphQLError val.graphQL = append(val.graphQL, err) // append the error to the existing slice in the context return ctx, nil } - return nil, fmt.Errorf("context does not contain GitHubCtxErrors") + return nil, errors.New(ErrContextMissingGitHubCtxErrors) } // NewGitHubAPIErrorResponse returns an mcp.NewToolResultError and retains the error in the context for access via middleware diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 29f776a05..148726649 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -447,6 +447,49 @@ func CreateRepository(getClient GetClientFn, t translations.TranslationHelperFun } // GetFileContents creates a tool to get the contents of a file or directory from a GitHub repository. +func tryRawContentFetch(ctx context.Context, getRawClient raw.GetRawClientFn, owner, repo, path string, opts *raw.ContentOpts) (*http.Response, error) { + rawClient, err := getRawClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub raw content client: %w", err) + } + return rawClient.GetRawContent(ctx, owner, repo, path, opts) +} + +func buildResourceURI(owner, repo, path, sha, ref string) (string, error) { + var resourceURI string + var err error + + switch { + case sha != "": + resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path) + case ref != "": + resourceURI, err = url.JoinPath("repo://", owner, repo, ref, "contents", path) + default: + resourceURI, err = url.JoinPath("repo://", owner, repo, "contents", path) + } + + if err != nil { + return "", fmt.Errorf("failed to create resource URI: %w", err) + } + return resourceURI, nil +} + +func createResourceContent(body []byte, contentType, uri string) (*mcp.CallToolResult, error) { + if strings.HasPrefix(contentType, "application") || strings.HasPrefix(contentType, "text") { + return mcp.NewToolResultResource("successfully downloaded text file", mcp.TextResourceContents{ + URI: uri, + Text: string(body), + MIMEType: contentType, + }), nil + } + + return mcp.NewToolResultResource("successfully downloaded binary file", mcp.BlobResourceContents{ + URI: uri, + Blob: base64.StdEncoding.EncodeToString(body), + MIMEType: contentType, + }), nil +} + func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_file_contents", mcp.WithDescription(t("TOOL_GET_FILE_CONTENTS_DESCRIPTION", "Get the contents of a file or directory from a GitHub repository")), @@ -523,12 +566,7 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t // If the path is (most likely) not to be a directory, we will first try to get the raw content from the GitHub raw content API. if path != "" && !strings.HasSuffix(path, "/") { - - rawClient, err := getRawClient(ctx) - if err != nil { - return mcp.NewToolResultError("failed to get GitHub raw content client"), nil - } - resp, err := rawClient.GetRawContent(ctx, owner, repo, path, rawOpts) + resp, err := tryRawContentFetch(ctx, getRawClient, owner, repo, path, rawOpts) if err != nil { return mcp.NewToolResultError("failed to get raw repository content"), nil } @@ -542,41 +580,14 @@ func GetFileContents(getClient GetClientFn, getRawClient raw.GetRawClientFn, t t if err != nil { return mcp.NewToolResultError("failed to read response body"), nil } - contentType := resp.Header.Get("Content-Type") - - var resourceURI string - switch { - case sha != "": - resourceURI, err = url.JoinPath("repo://", owner, repo, "sha", sha, "contents", path) - if err != nil { - return nil, fmt.Errorf("failed to create resource URI: %w", err) - } - case ref != "": - resourceURI, err = url.JoinPath("repo://", owner, repo, ref, "contents", path) - if err != nil { - return nil, fmt.Errorf("failed to create resource URI: %w", err) - } - default: - resourceURI, err = url.JoinPath("repo://", owner, repo, "contents", path) - if err != nil { - return nil, fmt.Errorf("failed to create resource URI: %w", err) - } - } - if strings.HasPrefix(contentType, "application") || strings.HasPrefix(contentType, "text") { - return mcp.NewToolResultResource("successfully downloaded text file", mcp.TextResourceContents{ - URI: resourceURI, - Text: string(body), - MIMEType: contentType, - }), nil + resourceURI, err := buildResourceURI(owner, repo, path, sha, ref) + if err != nil { + return nil, err } - return mcp.NewToolResultResource("successfully downloaded binary file", mcp.BlobResourceContents{ - URI: resourceURI, - Blob: base64.StdEncoding.EncodeToString(body), - MIMEType: contentType, - }), nil - + contentType := resp.Header.Get("Content-Type") + return createResourceContent(body, contentType, resourceURI) } } diff --git a/pkg/github/server.go b/pkg/github/server.go index 85d078f1b..75a7f5120 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + pkgerrors "github.com/github/github-mcp-server/pkg/errors" "github.com/google/go-github/v72/github" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" @@ -70,7 +71,7 @@ func RequiredParam[T comparable](r mcp.CallToolRequest, p string) (T, error) { // Check if the parameter is present in the request if _, ok := r.GetArguments()[p]; !ok { - return zero, fmt.Errorf("missing required parameter: %s", p) + return zero, fmt.Errorf(pkgerrors.ErrMissingRequiredParameter, p) } // Check if the parameter is of the expected type @@ -80,7 +81,7 @@ func RequiredParam[T comparable](r mcp.CallToolRequest, p string) (T, error) { } if val == zero { - return zero, fmt.Errorf("missing required parameter: %s", p) + return zero, fmt.Errorf(pkgerrors.ErrMissingRequiredParameter, p) } return val, nil