From 474d22596b012cb13ba9e003580fd635f031e7ff Mon Sep 17 00:00:00 2001 From: "Kristen T. Tran" Date: Fri, 6 Mar 2026 04:12:24 +0700 Subject: [PATCH] merge commit update changes (#8) * Stricter matching for github.com and ghe.com URLs * Gracefully handle numeric parameters passed as strings (#2130) * Gracefully handle numeric parameters passed as strings * Fix SHA validation in create_or_update_file (#2134) * Fix SHA validation in create_or_update_file * Doc update * Handle non-404 errors * Handle directory paths * Update instructions * fix: handle empty files in get_file_contents (#2042) 1. Empty (0-byte) files caused an unhandled error because the GitHub API returns null content with base64 encoding for them; GetContent() fails with "malformed response: base64 encoding of null content". Return empty text/plain content directly, bypassing decoding entirely. Co-authored-by: Ksenia Bobrova * Cline & Roo code installation guides (#2146) * Cline & Roo code installation guides * Update docs/installation-guides/install-cline.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Correctly wrap GraphQl error (#2149) --------- Co-authored-by: Ksenia Bobrova Co-authored-by: Jakub Janusz <32165716+kubajanusz@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- README.md | 2 +- docs/installation-guides/README.md | 4 + docs/installation-guides/install-cline.md | 56 +++++++ docs/installation-guides/install-roo-code.md | 58 +++++++ .../__toolsnaps__/create_or_update_file.snap | 4 +- pkg/github/copilot.go | 2 +- pkg/github/copilot_test.go | 109 ++++++++++++++ pkg/github/discussions.go | 4 +- pkg/github/discussions_test.go | 105 +++++++++++++ pkg/github/issues.go | 4 +- pkg/github/params.go | 111 ++++++++++++-- pkg/github/params_test.go | 141 +++++++++++++++++ pkg/github/pullrequests.go | 4 +- pkg/github/pullrequests_test.go | 114 ++++++++++++++ pkg/github/repositories.go | 133 ++++++++-------- pkg/github/repositories_test.go | 142 ++++++++++-------- pkg/utils/api.go | 4 +- pkg/utils/api_test.go | 75 +++++++++ 18 files changed, 920 insertions(+), 152 deletions(-) create mode 100644 docs/installation-guides/install-cline.md create mode 100644 docs/installation-guides/install-roo-code.md create mode 100644 pkg/utils/api_test.go diff --git a/README.md b/README.md index f83989a1e..1b926b132 100644 --- a/README.md +++ b/README.md @@ -1171,7 +1171,7 @@ The following sets of tools are available: - `owner`: Repository owner (username or organization) (string, required) - `path`: Path where to create/update the file (string, required) - `repo`: Repository name (string, required) - - `sha`: The blob SHA of the file being replaced. (string, optional) + - `sha`: The blob SHA of the file being replaced. Required if the file already exists. (string, optional) - **create_repository** - Create repository - **Required OAuth Scopes**: `repo` diff --git a/docs/installation-guides/README.md b/docs/installation-guides/README.md index 4a300e3f4..ab3aede36 100644 --- a/docs/installation-guides/README.md +++ b/docs/installation-guides/README.md @@ -7,9 +7,11 @@ This directory contains detailed installation instructions for the GitHub MCP Se - **[GitHub Copilot in other IDEs](install-other-copilot-ides.md)** - Installation for JetBrains, Visual Studio, Eclipse, and Xcode with GitHub Copilot - **[Antigravity](install-antigravity.md)** - Installation for Google Antigravity IDE - **[Claude Applications](install-claude.md)** - Installation guide for Claude Web, Claude Desktop and Claude Code CLI +- **[Cline](install-cline.md)** - Installation guide for Cline - **[Cursor](install-cursor.md)** - Installation guide for Cursor IDE - **[Google Gemini CLI](install-gemini-cli.md)** - Installation guide for Google Gemini CLI - **[OpenAI Codex](install-codex.md)** - Installation guide for OpenAI Codex +- **[Roo Code](install-roo-code.md)** - Installation guide for Roo Code - **[Windsurf](install-windsurf.md)** - Installation guide for Windsurf IDE ## Support by Host Application @@ -23,8 +25,10 @@ This directory contains detailed installation instructions for the GitHub MCP Se | Copilot in JetBrains | ✅ | ✅ Full (OAuth + PAT) | Local: Docker or Go build, GitHub PAT
Remote: JetBrains Copilot Extension v1.5.53+ | Easy | | Claude Code | ✅ | ✅ PAT + ❌ No OAuth| GitHub MCP Server binary or remote URL, GitHub PAT | Easy | | Claude Desktop | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Moderate | +| Cline | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Easy | | Cursor | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Easy | | Google Gemini CLI | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Easy | +| Roo Code | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Easy | | Windsurf | ✅ | ✅ PAT + ❌ No OAuth | Docker or Go build, GitHub PAT | Easy | | Copilot in Xcode | ✅ | ✅ Full (OAuth + PAT) | Local: Docker or Go build, GitHub PAT
Remote: Copilot for Xcode 0.41.0+ | Easy | | Copilot in Eclipse | ✅ | ✅ Full (OAuth + PAT) | Local: Docker or Go build, GitHub PAT
Remote: Eclipse Plug-in for Copilot 0.10.0+ | Easy | diff --git a/docs/installation-guides/install-cline.md b/docs/installation-guides/install-cline.md new file mode 100644 index 000000000..6bc643cb6 --- /dev/null +++ b/docs/installation-guides/install-cline.md @@ -0,0 +1,56 @@ +# Install GitHub MCP Server in Cline + +[Cline](https://github.com/cline/cline) is an AI coding assistant that runs in VS Code-compatible editors (VS Code, Cursor, Windsurf, etc.). For general setup information (prerequisites, Docker installation, security best practices), see the [Installation Guides README](./README.md). + +## Remote Server + +Cline stores MCP settings in `cline_mcp_settings.json`. To edit it, click the Cline icon in your editor's sidebar, open the menu in the top right corner of the Cline panel, and select **"MCP Servers"**. You can add a remote server through the **"Remote Servers"** tab, or click **"Configure MCP Servers"** to edit the JSON directly. + +```json +{ + "mcpServers": { + "github": { + "url": "https://api.githubcopilot.com/mcp/", + "type": "streamableHttp", + "disabled": false, + "headers": { + "Authorization": "Bearer " + }, + "autoApprove": [] + } + } +} +``` + +Replace `YOUR_GITHUB_PAT` with your [GitHub Personal Access Token](https://github.com/settings/tokens). To customize toolsets, add server-side headers like `X-MCP-Toolsets` or `X-MCP-Readonly` to the `headers` object — see [Server Configuration Guide](../server-configuration.md). + +> **Important:** The transport type must be `"streamableHttp"` (camelCase, no hyphen). Using `"streamable-http"` or omitting the type will cause Cline to fall back to SSE, resulting in a `405` error. + +## Local Server (Docker) + +1. Click the Cline icon in your editor's sidebar (or open the command palette and search for "Cline"), then click the **MCP Servers** icon (server stack icon at the top of the Cline panel), and click **"Configure MCP Servers"** to open `cline_mcp_settings.json`. +2. Add the configuration below, replacing `YOUR_GITHUB_PAT` with your [GitHub Personal Access Token](https://github.com/settings/tokens). + +```json +{ + "mcpServers": { + "github": { + "command": "docker", + "args": [ + "run", "-i", "--rm", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", + "ghcr.io/github/github-mcp-server" + ], + "env": { + "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_PAT" + } + } + } +} +``` + +## Troubleshooting + +- **SSE error 405 with remote server**: Ensure `"type"` is set to `"streamableHttp"` (camelCase, no hyphen) in `cline_mcp_settings.json`. Using `"streamable-http"` or omitting `"type"` causes Cline to fall back to SSE, which this server does not support. +- **Authentication failures**: Verify your PAT has the required scopes +- **Docker issues**: Ensure Docker Desktop is installed and running diff --git a/docs/installation-guides/install-roo-code.md b/docs/installation-guides/install-roo-code.md new file mode 100644 index 000000000..77513fb55 --- /dev/null +++ b/docs/installation-guides/install-roo-code.md @@ -0,0 +1,58 @@ +# Install GitHub MCP Server in Roo Code + +[Roo Code](https://github.com/RooCodeInc/Roo-Code) is an AI coding assistant that runs in VS Code-compatible editors (VS Code, Cursor, Windsurf, etc.). For general setup information (prerequisites, Docker installation, security best practices), see the [Installation Guides README](./README.md). + +## Remote Server + +### Step-by-step setup + +1. Click the **Roo Code icon** in your editor's sidebar to open the Roo Code pane +2. Click the **gear icon** (⚙️) in the top navigation of the Roo Code pane, then click on **"MCP Servers"** icon on the left. +3. Scroll to the bottom and click **"Edit Global MCP"** (for all projects) or **"Edit Project MCP"** (for the current project only) +4. Add the configuration below to the opened file (`mcp_settings.json` or `.roo/mcp.json`) +5. Replace `YOUR_GITHUB_PAT` with your [GitHub Personal Access Token](https://github.com/settings/tokens) +6. Save the file — the server should connect automatically + +```json +{ + "mcpServers": { + "github": { + "type": "streamable-http", + "url": "https://api.githubcopilot.com/mcp/", + "headers": { + "Authorization": "Bearer YOUR_GITHUB_PAT" + } + } + } +} +``` + +> **Important:** The `type` must be `"streamable-http"` (with hyphen). Using `"http"` or omitting the type will fail. + +To customize toolsets, add server-side headers like `X-MCP-Toolsets` or `X-MCP-Readonly` to the `headers` object — see [Server Configuration Guide](../server-configuration.md). + +## Local Server (Docker) + +```json +{ + "mcpServers": { + "github": { + "command": "docker", + "args": [ + "run", "-i", "--rm", + "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", + "ghcr.io/github/github-mcp-server" + ], + "env": { + "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_GITHUB_PAT" + } + } + } +} +``` + +## Troubleshooting + +- **Connection failures**: Ensure `type` is `streamable-http`, not `http` +- **Authentication failures**: Verify PAT is prefixed with `Bearer ` in the `Authorization` header +- **Docker issues**: Ensure Docker Desktop is running diff --git a/pkg/github/__toolsnaps__/create_or_update_file.snap b/pkg/github/__toolsnaps__/create_or_update_file.snap index 9d28c8085..e6900c905 100644 --- a/pkg/github/__toolsnaps__/create_or_update_file.snap +++ b/pkg/github/__toolsnaps__/create_or_update_file.snap @@ -2,7 +2,7 @@ "annotations": { "title": "Create or update file" }, - "description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit ls-tree HEAD \u003cpath to file\u003e\n\nIf the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval.\n", + "description": "Create or update a single file in a GitHub repository. \nIf updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations.\n\nIn order to obtain the SHA of original file version before updating, use the following git command:\ngit rev-parse \u003cbranch\u003e:\u003cpath to file\u003e\n\nSHA MUST be provided for existing file updates.\n", "inputSchema": { "properties": { "branch": { @@ -30,7 +30,7 @@ "type": "string" }, "sha": { - "description": "The blob SHA of the file being replaced.", + "description": "The blob SHA of the file being replaced. Required if the file already exists.", "type": "string" } }, diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 525a58c69..d95357e73 100644 --- a/pkg/github/copilot.go +++ b/pkg/github/copilot.go @@ -209,7 +209,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server BaseRef string `mapstructure:"base_ref"` CustomInstructions string `mapstructure:"custom_instructions"` } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/copilot_test.go b/pkg/github/copilot_test.go index 6b1e7c990..0a1d5ef3b 100644 --- a/pkg/github/copilot_test.go +++ b/pkg/github/copilot_test.go @@ -165,6 +165,115 @@ func TestAssignCopilotToIssue(t *testing.T) { ), ), }, + { + name: "successful assignment with string issue_number", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": "123", // Some MCP clients send numeric values as strings + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + SuggestedActors struct { + Nodes []struct { + Bot struct { + ID githubv4.ID + Login githubv4.String + TypeName string `graphql:"__typename"` + } `graphql:"... on Bot"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"suggestedActors(first: 100, after: $endCursor, capabilities: CAN_BE_ASSIGNED)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "endCursor": (*githubv4.String)(nil), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "suggestedActors": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("copilot-swe-agent-id"), + "login": githubv4.String("copilot-swe-agent"), + "__typename": "Bot", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + ID githubv4.ID + Issue struct { + ID githubv4.ID + Assignees struct { + Nodes []struct { + ID githubv4.ID + } + } `graphql:"assignees(first: 100)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "name": githubv4.String("repo"), + "number": githubv4.Int(123), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "id": githubv4.ID("test-repo-id"), + "issue": map[string]any{ + "id": githubv4.ID("test-issue-id"), + "assignees": map[string]any{ + "nodes": []any{}, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + UpdateIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + } + } `graphql:"updateIssue(input: $input)"` + }{}, + UpdateIssueInput{ + ID: githubv4.ID("test-issue-id"), + AssigneeIDs: []githubv4.ID{githubv4.ID("copilot-swe-agent-id")}, + AgentAssignment: &AgentAssignmentInput{ + BaseRef: nil, + CustomAgent: ptrGitHubv4String(""), + CustomInstructions: ptrGitHubv4String(""), + TargetRepositoryID: githubv4.ID("test-repo-id"), + }, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateIssue": map[string]any{ + "issue": map[string]any{ + "id": githubv4.ID("test-issue-id"), + "number": githubv4.Int(123), + "url": githubv4.String("https://github.com/owner/repo/issues/123"), + }, + }, + }), + ), + ), + }, { name: "successful assignment when there are existing assignees", requestArgs: map[string]any{ diff --git a/pkg/github/discussions.go b/pkg/github/discussions.go index 6971bab07..700560b47 100644 --- a/pkg/github/discussions.go +++ b/pkg/github/discussions.go @@ -313,7 +313,7 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool { Repo string DiscussionNumber int32 } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetGQLClient(ctx) @@ -417,7 +417,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve Repo string DiscussionNumber int32 } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/discussions_test.go b/pkg/github/discussions_test.go index 998a6471b..692ef2ec8 100644 --- a/pkg/github/discussions_test.go +++ b/pkg/github/discussions_test.go @@ -590,6 +590,50 @@ func Test_GetDiscussion(t *testing.T) { } } +func Test_GetDiscussionWithStringNumber(t *testing.T) { + // Test that WeakDecode handles string discussionNumber from MCP clients + toolDef := GetDiscussion(translations.NullTranslationHelper) + + qGetDiscussion := "query($discussionNumber:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){number,title,body,createdAt,closed,isAnswered,answerChosenAt,url,category{name}}}}" + + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + } + + matcher := githubv4mock.NewQueryMatcher(qGetDiscussion, vars, githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{"discussion": map[string]any{ + "number": 1, + "title": "Test Discussion Title", + "body": "This is a test discussion", + "url": "https://github.com/owner/repo/discussions/1", + "createdAt": "2025-04-25T12:00:00Z", + "closed": false, + "isAnswered": false, + "category": map[string]any{"name": "General"}, + }}, + })) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + // Send discussionNumber as a string instead of a number + reqParams := map[string]any{"owner": "owner", "repo": "repo", "discussionNumber": "1"} + req := createMCPRequest(reqParams) + res, err := handler(ContextWithDeps(context.Background(), deps), &req) + require.NoError(t, err) + + text := getTextResult(t, res).Text + require.False(t, res.IsError, "expected no error, got: %s", text) + + var out map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &out)) + assert.Equal(t, float64(1), out["number"]) + assert.Equal(t, "Test Discussion Title", out["title"]) +} + func Test_GetDiscussionComments(t *testing.T) { // Verify tool definition and schema toolDef := GetDiscussionComments(translations.NullTranslationHelper) @@ -675,6 +719,67 @@ func Test_GetDiscussionComments(t *testing.T) { } } +func Test_GetDiscussionCommentsWithStringNumber(t *testing.T) { + // Test that WeakDecode handles string discussionNumber from MCP clients + toolDef := GetDiscussionComments(translations.NullTranslationHelper) + + qGetComments := "query($after:String$discussionNumber:Int!$first:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){discussion(number: $discussionNumber){comments(first: $first, after: $after){nodes{body},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount}}}}" + + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": float64(1), + "first": float64(30), + "after": (*string)(nil), + } + + mockResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "discussion": map[string]any{ + "comments": map[string]any{ + "nodes": []map[string]any{ + {"body": "First comment"}, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 1, + }, + }, + }, + }) + matcher := githubv4mock.NewQueryMatcher(qGetComments, vars, mockResponse) + httpClient := githubv4mock.NewMockedHTTPClient(matcher) + gqlClient := githubv4.NewClient(httpClient) + deps := BaseDeps{GQLClient: gqlClient} + handler := toolDef.Handler(deps) + + // Send discussionNumber as a string instead of a number + reqParams := map[string]any{ + "owner": "owner", + "repo": "repo", + "discussionNumber": "1", + } + request := createMCPRequest(reqParams) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + require.False(t, result.IsError, "expected no error, got: %s", textContent.Text) + + var out struct { + Comments []map[string]any `json:"comments"` + TotalCount int `json:"totalCount"` + } + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &out)) + assert.Len(t, out.Comments, 1) + assert.Equal(t, "First comment", out.Comments[0]["body"]) +} + func Test_ListDiscussionCategories(t *testing.T) { toolDef := ListDiscussionCategories(translations.NullTranslationHelper) tool := toolDef.Tool diff --git a/pkg/github/issues.go b/pkg/github/issues.go index b5bc4ebb8..9709a852c 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -62,7 +62,7 @@ func fetchIssueIDs(ctx context.Context, gqlClient *githubv4.Client, owner, repo } if err := gqlClient.Query(ctx, &query, vars); err != nil { - return "", "", fmt.Errorf("failed to get issue ID") + return "", "", fmt.Errorf("failed to get issue ID: %w", err) } return query.Repository.Issue.ID, "", nil @@ -84,7 +84,7 @@ func fetchIssueIDs(ctx context.Context, gqlClient *githubv4.Client, owner, repo vars["duplicateOf"] = githubv4.Int(duplicateOf) // #nosec G115 - issue numbers are always small positive integers if err := gqlClient.Query(ctx, &query, vars); err != nil { - return "", "", fmt.Errorf("failed to get issue ID") + return "", "", fmt.Errorf("failed to get issue ID: %w", err) } return query.Repository.Issue.ID, query.Repository.DuplicateIssue.ID, nil diff --git a/pkg/github/params.go b/pkg/github/params.go index 0dac1773f..1b45d61bd 100644 --- a/pkg/github/params.go +++ b/pkg/github/params.go @@ -3,6 +3,7 @@ package github import ( "errors" "fmt" + "math" "strconv" "github.com/google/go-github/v82/github" @@ -39,6 +40,66 @@ func isAcceptedError(err error) bool { return errors.As(err, &acceptedError) } +// toInt converts a value to int, handling both float64 and string representations. +// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf, +// fractional values, and values outside the int range. +func toInt(val any) (int, error) { + var f float64 + switch v := val.(type) { + case float64: + f = v + case string: + var err error + f, err = strconv.ParseFloat(v, 64) + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + default: + return 0, fmt.Errorf("expected number, got %T", val) + } + if math.IsNaN(f) || math.IsInf(f, 0) { + return 0, fmt.Errorf("non-finite numeric value") + } + if f != math.Trunc(f) { + return 0, fmt.Errorf("non-integer numeric value: %v", f) + } + if f > math.MaxInt || f < math.MinInt { + return 0, fmt.Errorf("numeric value out of int range: %v", f) + } + return int(f), nil +} + +// toInt64 converts a value to int64, handling both float64 and string representations. +// Some MCP clients send numeric values as strings. It rejects NaN, ±Inf, +// fractional values, and values that lose precision in the float64→int64 conversion. +func toInt64(val any) (int64, error) { + var f float64 + switch v := val.(type) { + case float64: + f = v + case string: + var err error + f, err = strconv.ParseFloat(v, 64) + if err != nil { + return 0, fmt.Errorf("invalid numeric value: %s", v) + } + default: + return 0, fmt.Errorf("expected number, got %T", val) + } + if math.IsNaN(f) || math.IsInf(f, 0) { + return 0, fmt.Errorf("non-finite numeric value") + } + if f != math.Trunc(f) { + return 0, fmt.Errorf("non-integer numeric value: %v", f) + } + result := int64(f) + // Check round-trip to detect precision loss for large int64 values + if float64(result) != f { + return 0, fmt.Errorf("numeric value %v is too large to fit in int64", f) + } + return result, nil +} + // RequiredParam is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. @@ -68,33 +129,47 @@ func RequiredParam[T comparable](args map[string]any, p string) (T, error) { // RequiredInt is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. -// 2. Checks if the parameter is of the expected type. +// 2. Checks if the parameter is of the expected type (float64 or numeric string). // 3. Checks if the parameter is not empty, i.e: non-zero value func RequiredInt(args map[string]any, p string) (int, error) { - v, err := RequiredParam[float64](args, p) + v, ok := args[p] + if !ok { + return 0, fmt.Errorf("missing required parameter: %s", p) + } + + result, err := toInt(v) if err != nil { - return 0, err + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) + } + + if result == 0 { + return 0, fmt.Errorf("missing required parameter: %s", p) } - return int(v), nil + + return result, nil } // RequiredBigInt is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request. -// 2. Checks if the parameter is of the expected type (float64). +// 2. Checks if the parameter is of the expected type (float64 or numeric string). // 3. Checks if the parameter is not empty, i.e: non-zero value. // 4. Validates that the float64 value can be safely converted to int64 without truncation. func RequiredBigInt(args map[string]any, p string) (int64, error) { - v, err := RequiredParam[float64](args, p) + val, ok := args[p] + if !ok { + return 0, fmt.Errorf("missing required parameter: %s", p) + } + + result, err := toInt64(val) if err != nil { - return 0, err + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) } - result := int64(v) - // Check if converting back produces the same value to avoid silent truncation - if float64(result) != v { - return 0, fmt.Errorf("parameter %s value %f is too large to fit in int64", p, v) + if result == 0 { + return 0, fmt.Errorf("missing required parameter: %s", p) } + return result, nil } @@ -121,13 +196,19 @@ func OptionalParam[T any](args map[string]any, p string) (T, error) { // OptionalIntParam is a helper function that can be used to fetch a requested parameter from the request. // It does the following checks: // 1. Checks if the parameter is present in the request, if not, it returns its zero-value -// 2. If it is present, it checks if the parameter is of the expected type and returns it +// 2. If it is present, it checks if the parameter is of the expected type (float64 or numeric string) and returns it func OptionalIntParam(args map[string]any, p string) (int, error) { - v, err := OptionalParam[float64](args, p) + val, ok := args[p] + if !ok { + return 0, nil + } + + result, err := toInt(val) if err != nil { - return 0, err + return 0, fmt.Errorf("parameter %s is not a valid number: %w", p, err) } - return int(v), nil + + return result, nil } // OptionalIntParamWithDefault is a helper function that can be used to fetch a requested parameter from the request diff --git a/pkg/github/params_test.go b/pkg/github/params_test.go index 5c989d55a..2254b737e 100644 --- a/pkg/github/params_test.go +++ b/pkg/github/params_test.go @@ -2,6 +2,7 @@ package github import ( "fmt" + "math" "testing" "github.com/google/go-github/v82/github" @@ -163,6 +164,13 @@ func Test_RequiredInt(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -170,6 +178,13 @@ func Test_RequiredInt(t *testing.T) { expected: 0, expectError: true, }, + { + name: "zero string parameter", + params: map[string]any{"count": "0"}, + paramName: "count", + expected: 0, + expectError: true, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -177,6 +192,69 @@ func Test_RequiredInt(t *testing.T) { expected: 0, expectError: true, }, + { + name: "boolean type parameter", + params: map[string]any{"count": true}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "NaN string", + params: map[string]any{"count": "NaN"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "Inf string", + params: map[string]any{"count": "Inf"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "negative Inf string", + params: map[string]any{"count": "-Inf"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional string", + params: map[string]any{"count": "1.5"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional float64", + params: map[string]any{"count": float64(1.5)}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "NaN float64", + params: map[string]any{"count": math.NaN()}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "Inf float64", + params: map[string]any{"count": math.Inf(1)}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "MaxFloat64", + params: map[string]any{"count": math.MaxFloat64}, + paramName: "count", + expected: 0, + expectError: true, + }, } for _, tc := range tests { @@ -207,6 +285,13 @@ func Test_OptionalIntParam(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -221,6 +306,13 @@ func Test_OptionalIntParam(t *testing.T) { expected: 0, expectError: false, }, + { + name: "zero string value", + params: map[string]any{"count": "0"}, + paramName: "count", + expected: 0, + expectError: false, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -228,6 +320,27 @@ func Test_OptionalIntParam(t *testing.T) { expected: 0, expectError: true, }, + { + name: "NaN string", + params: map[string]any{"count": "NaN"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional string", + params: map[string]any{"count": "1.5"}, + paramName: "count", + expected: 0, + expectError: true, + }, + { + name: "fractional float64", + params: map[string]any{"count": float64(1.5)}, + paramName: "count", + expected: 0, + expectError: true, + }, } for _, tc := range tests { @@ -261,6 +374,14 @@ func Test_OptionalNumberParamWithDefault(t *testing.T) { expected: 42, expectError: false, }, + { + name: "valid string number parameter", + params: map[string]any{"count": "42"}, + paramName: "count", + defaultVal: 10, + expected: 42, + expectError: false, + }, { name: "missing parameter", params: map[string]any{}, @@ -277,6 +398,14 @@ func Test_OptionalNumberParamWithDefault(t *testing.T) { expected: 10, expectError: false, }, + { + name: "zero string value uses default", + params: map[string]any{"count": "0"}, + paramName: "count", + defaultVal: 10, + expected: 10, + expectError: false, + }, { name: "wrong type parameter", params: map[string]any{"count": "not-a-number"}, @@ -486,6 +615,18 @@ func TestOptionalPaginationParams(t *testing.T) { expected: PaginationParams{}, expectError: true, }, + { + name: "string page and perPage parameters", + params: map[string]any{ + "page": "3", + "perPage": "25", + }, + expected: PaginationParams{ + Page: 3, + PerPage: 25, + }, + expectError: false, + }, } for _, tc := range tests { diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 7eb9c6d64..e5e0855ea 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1570,7 +1570,7 @@ Available methods: []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { var params PullRequestReviewWriteParams - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -1905,7 +1905,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S StartLine *int32 StartSide *string } - if err := mapstructure.Decode(args, ¶ms); err != nil { + if err := mapstructure.WeakDecode(args, ¶ms); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 4bb31e541..537577329 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2470,6 +2470,61 @@ func TestCreateAndSubmitPullRequestReview(t *testing.T) { }, expectToolError: false, }, + { + name: "successful review creation with string pullNumber", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + }, + }, + }, + ), + ), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReview struct { + PullRequestReview struct { + ID githubv4.ID + } + } `graphql:"addPullRequestReview(input: $input)"` + }{}, + githubv4.AddPullRequestReviewInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + Body: githubv4.NewString("This is a test review"), + Event: githubv4mock.Ptr(githubv4.PullRequestReviewEventComment), + CommitOID: githubv4.NewGitObjectID("abcd1234"), + }, + nil, + githubv4mock.DataResponse(map[string]any{}), + ), + ), + requestArgs: map[string]any{ + "method": "create", + "owner": "owner", + "repo": "repo", + "pullNumber": "42", // Some MCP clients send numeric values as strings + "body": "This is a test review", + "event": "COMMENT", + "commitID": "abcd1234", + }, + expectToolError: false, + }, { name: "failure to get pull request", mockedClient: githubv4mock.NewMockedHTTPClient( @@ -2873,6 +2928,65 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { ), ), }, + { + name: "successful line comment with string pullNumber and line", + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": "42", // Some MCP clients send numeric values as strings + "path": "file.go", + "body": "This is a test comment", + "subjectType": "LINE", + "line": "10", // string line number + "side": "RIGHT", + "startLine": "5", // string startLine + "startSide": "RIGHT", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + viewerQuery("williammartin"), + getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ + author: "williammartin", + owner: "owner", + repo: "repo", + prNum: 42, + + reviews: []getLatestPendingReviewQueryReview{ + { + id: "PR_kwDODKw3uc6WYN1T", + state: "PENDING", + url: "https://github.com/owner/repo/pull/42", + }, + }, + }), + githubv4mock.NewMutationMatcher( + struct { + AddPullRequestReviewThread struct { + Thread struct { + ID githubv4.String + } + } `graphql:"addPullRequestReviewThread(input: $input)"` + }{}, + githubv4.AddPullRequestReviewThreadInput{ + Path: githubv4.String("file.go"), + Body: githubv4.String("This is a test comment"), + SubjectType: githubv4mock.Ptr(githubv4.PullRequestReviewThreadSubjectTypeLine), + Line: githubv4.NewInt(10), + Side: githubv4mock.Ptr(githubv4.DiffSideRight), + StartLine: githubv4.NewInt(5), + StartSide: githubv4mock.Ptr(githubv4.DiffSideRight), + PullRequestReviewID: githubv4.NewID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "addPullRequestReviewThread": map[string]any{ + "thread": map[string]any{ + "id": "MDEyOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTIzNDU2", + }, + }, + }), + ), + ), + }, { name: "thread ID is nil - invalid line number", requestArgs: map[string]any{ diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index a236609bc..9376ddad4 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net/http" - "net/url" "strings" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -323,9 +322,9 @@ func CreateOrUpdateFile(t translations.TranslationHelperFunc) inventory.ServerTo If updating, you should provide the SHA of the file you want to update. Use this tool to create or update a file in a GitHub repository remotely; do not use it for local file operations. In order to obtain the SHA of original file version before updating, use the following git command: -git ls-tree HEAD +git rev-parse : -If the SHA is not provided, the tool will attempt to acquire it by fetching the current file contents from the repository, which may lead to rewriting latest committed changes if the file has changed since last retrieval. +SHA MUST be provided for existing file updates. `), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_CREATE_OR_UPDATE_FILE_USER_TITLE", "Create or update file"), @@ -360,7 +359,7 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the }, "sha": { Type: "string", - Description: "The blob SHA of the file being replaced.", + Description: "The blob SHA of the file being replaced. Required if the file already exists.", }, }, Required: []string{"owner", "repo", "path", "content", "message", "branch"}, @@ -420,55 +419,68 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the path = strings.TrimPrefix(path, "/") - // SHA validation using conditional HEAD request (efficient - no body transfer) - var previousSHA string - contentURL := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(path)) - if branch != "" { - contentURL += "?ref=" + url.QueryEscape(branch) - } + // SHA validation using Contents API to fetch current file metadata (blob SHA) + getOpts := &github.RepositoryContentGetOptions{Ref: branch} if sha != "" { // User provided SHA - validate it's still current - req, err := client.NewRequest("HEAD", contentURL, nil) - if err == nil { - req.Header.Set("If-None-Match", fmt.Sprintf(`"%s"`, sha)) - resp, _ := client.Do(ctx, req, nil) - if resp != nil { - defer resp.Body.Close() - - switch resp.StatusCode { - case http.StatusNotModified: - // SHA matches current - proceed - opts.SHA = github.Ptr(sha) - case http.StatusOK: - // SHA is stale - reject with current SHA so user can check diff - currentSHA := strings.Trim(resp.Header.Get("ETag"), `"`) - return utils.NewToolResultError(fmt.Sprintf( - "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ - "Use get_file_contents or compare commits to review changes before updating.", - sha, currentSHA)), nil, nil - case http.StatusNotFound: - // File doesn't exist - this is a create, ignore provided SHA - } + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + // 404 means file doesn't exist - proceed (new file creation) + // Any other error (403, 500, network) should be surfaced + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to verify file SHA", + respCheck, + getErr, + ), nil, nil + } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + currentSHA := existingFile.GetSHA() + if currentSHA != sha { + return utils.NewToolResultError(fmt.Sprintf( + "SHA mismatch: provided SHA %s is stale. Current file SHA is %s. "+ + "Pull the latest changes and use git rev-parse %s:%s to get the current SHA.", + sha, currentSHA, branch, path)), nil, nil } } } else { - // No SHA provided - check if file exists to warn about blind update - req, err := client.NewRequest("HEAD", contentURL, nil) - if err == nil { - resp, _ := client.Do(ctx, req, nil) - if resp != nil { - defer resp.Body.Close() - if resp.StatusCode == http.StatusOK { - previousSHA = strings.Trim(resp.Header.Get("ETag"), `"`) - } - // 404 = new file, no previous SHA needed + // No SHA provided - check if file already exists + existingFile, dirContent, respCheck, getErr := client.Repositories.GetContents(ctx, owner, repo, path, getOpts) + if respCheck != nil { + _ = respCheck.Body.Close() + } + switch { + case getErr != nil: + // 404 means file doesn't exist - proceed with creation + // Any other error (403, 500, network) should be surfaced + if respCheck == nil || respCheck.StatusCode != http.StatusNotFound { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to check if file exists", + respCheck, + getErr, + ), nil, nil } + case dirContent != nil: + return utils.NewToolResultError(fmt.Sprintf( + "Path %s is a directory, not a file. This tool only works with files.", + path)), nil, nil + case existingFile != nil: + // File exists but no SHA was provided - reject to prevent blind overwrites + return utils.NewToolResultError(fmt.Sprintf( + "File already exists at %s. You must provide the current file's SHA when updating. "+ + "Use git rev-parse %s:%s to get the blob SHA, then retry with the sha parameter.", + path, branch, path)), nil, nil } - } - - if previousSHA != "" { - opts.SHA = github.Ptr(previousSHA) + // If file not found, no previous SHA needed (new file creation) } fileContent, resp, err := client.Repositories.CreateFile(ctx, owner, repo, path, opts) @@ -491,23 +503,6 @@ If the SHA is not provided, the tool will attempt to acquire it by fetching the minimalResponse := convertToMinimalFileContentResponse(fileContent) - // Warn if file was updated without SHA validation (blind update) - if sha == "" && previousSHA != "" { - warning, err := json.Marshal(minimalResponse) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal response: %w", err) - } - return utils.NewToolResultText(fmt.Sprintf( - "Warning: File updated without SHA validation. Previous file SHA was %s. "+ - `Verify no unintended changes were overwritten: -1. Extract the SHA of the local version using git ls-tree HEAD %s. -2. Compare with the previous SHA above. -3. Revert changes if shas do not match. - -%s`, - previousSHA, path, string(warning))), nil, nil - } - return MarshalledTextResult(minimalResponse), nil, nil }, ) @@ -731,6 +726,20 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool successNote = fmt.Sprintf(" Note: the provided ref '%s' does not exist, default branch '%s' was used instead.", originalRef, rawOpts.Ref) } + // Empty files (0 bytes) have no content to decode; return + // them directly as empty text to avoid errors from + // GetContent when the API returns null content with a + // base64 encoding field, and to avoid DetectContentType + // misclassifying them as binary. + if fileSize == 0 { + result := &mcp.ResourceContents{ + URI: resourceURI, + Text: "", + MIMEType: "text/plain", + } + return utils.NewToolResultResource(fmt.Sprintf("successfully downloaded empty file (SHA: %s)%s", fileSHA, successNote), result), nil, nil + } + // For files >= 1MB, return a ResourceLink instead of content const maxContentSize = 1024 * 1024 // 1MB if fileSize >= maxContentSize { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index fdb5780c2..ae2ece0f6 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -351,6 +351,40 @@ func Test_GetFileContents(t *testing.T) { Title: "File: large-file.bin", }, }, + { + name: "successful empty file content fetch", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposGitRefByOwnerByRepoByRef: mockResponse(t, http.StatusOK, "{\"ref\": \"refs/heads/main\", \"object\": {\"sha\": \"\"}}"), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, "{\"name\": \"repo\", \"default_branch\": \"main\"}"), + GetReposContentsByOwnerByRepoByPath: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + fileContent := &github.RepositoryContent{ + Name: github.Ptr(".gitkeep"), + Path: github.Ptr(".gitkeep"), + SHA: github.Ptr("empty123"), + Type: github.Ptr("file"), + Content: nil, + Size: github.Ptr(0), + Encoding: github.Ptr("base64"), + } + contentBytes, _ := json.Marshal(fileContent) + _, _ = w.Write(contentBytes) + }, + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "path": ".gitkeep", + "ref": "refs/heads/main", + }, + expectError: false, + expectedResult: mcp.ResourceContents{ + URI: "repo://owner/repo/refs/heads/main/contents/.gitkeep", + Text: "", + MIMEType: "text/plain", + }, + expectedMsg: "successfully downloaded empty file", + }, { name: "content fetch fails", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -1157,6 +1191,14 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "successful file update with SHA", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ "message": "Update example file", "content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==", // Base64 encoded content @@ -1210,26 +1252,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedErrMsg: "failed to create/update file", }, { - name: "sha validation - current sha matches (304 Not Modified)", + name: "sha validation - current sha matches", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, req *http.Request) { - ifNoneMatch := req.Header.Get("If-None-Match") - if ifNoneMatch == `"abc123def456"` { - w.WriteHeader(http.StatusNotModified) - } else { - w.WriteHeader(http.StatusOK) - w.Header().Set("ETag", `"abc123def456"`) - } - }, - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, req *http.Request) { - ifNoneMatch := req.Header.Get("If-None-Match") - if ifNoneMatch == `"abc123def456"` { - w.WriteHeader(http.StatusNotModified) - } else { - w.WriteHeader(http.StatusOK) - w.Header().Set("ETag", `"abc123def456"`) - } - }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("abc123def456"), + Type: github.Ptr("file"), + }), PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ "message": "Update example file", "content": "IyBVcGRhdGVkIEV4YW1wbGUKClRoaXMgZmlsZSBoYXMgYmVlbiB1cGRhdGVkLg==", @@ -1260,16 +1292,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent: mockFileResponse, }, { - name: "sha validation - stale sha detected (200 OK with different ETag)", + name: "sha validation - stale sha detected", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"newsha999888"`) - w.WriteHeader(http.StatusOK) - }, - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"newsha999888"`) - w.WriteHeader(http.StatusOK) - }, + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("newsha999888"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("newsha999888"), + Type: github.Ptr("file"), + }), }), requestArgs: map[string]any{ "owner": "owner", @@ -1286,7 +1318,10 @@ func Test_CreateOrUpdateFile(t *testing.T) { { name: "sha validation - file doesn't exist (404), proceed with create", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + "GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + "GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }, PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ @@ -1297,9 +1332,6 @@ func Test_CreateOrUpdateFile(t *testing.T) { }).andThen( mockResponse(t, http.StatusCreated, mockFileResponse), ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - }, "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ "message": "Create new file", "content": "IyBOZXcgRmlsZQoKVGhpcyBpcyBhIG5ldyBmaWxlLg==", @@ -1322,32 +1354,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { expectedContent: mockFileResponse, }, { - name: "no sha provided - file exists, returns warning", + name: "no sha provided - file exists, rejects update", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"existing123"`) - w.WriteHeader(http.StatusOK) - }, - PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ - "message": "Update without SHA", - "content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==", - "branch": "main", - "sha": "existing123", // SHA is automatically added from ETag - }).andThen( - mockResponse(t, http.StatusOK, mockFileResponse), - ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("ETag", `"existing123"`) - w.WriteHeader(http.StatusOK) - }, - "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ - "message": "Update without SHA", - "content": "IyBVcGRhdGVkCgpVcGRhdGVkIHdpdGhvdXQgU0hBLg==", - "branch": "main", - "sha": "existing123", // SHA is automatically added from ETag - }).andThen( - mockResponse(t, http.StatusOK, mockFileResponse), - ), + "GET /repos/owner/repo/contents/docs/example.md": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("existing123"), + Type: github.Ptr("file"), + }), + "GET /repos/{owner}/{repo}/contents/{path:.*}": mockResponse(t, http.StatusOK, &github.RepositoryContent{ + SHA: github.Ptr("existing123"), + Type: github.Ptr("file"), + }), }), requestArgs: map[string]any{ "owner": "owner", @@ -1357,13 +1373,16 @@ func Test_CreateOrUpdateFile(t *testing.T) { "message": "Update without SHA", "branch": "main", }, - expectError: false, - expectedErrMsg: "Warning: File updated without SHA validation. Previous file SHA was existing123", + expectError: true, + expectedErrMsg: "File already exists at docs/example.md", }, { name: "no sha provided - file doesn't exist, no warning", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - "HEAD /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + "GET /repos/owner/repo/contents/docs/example.md": func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + "GET /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }, PutReposContentsByOwnerByRepoByPath: expectRequestBody(t, map[string]any{ @@ -1373,9 +1392,6 @@ func Test_CreateOrUpdateFile(t *testing.T) { }).andThen( mockResponse(t, http.StatusCreated, mockFileResponse), ), - "HEAD /repos/{owner}/{repo}/contents/{path:.*}": func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - }, "PUT /repos/{owner}/{repo}/contents/{path:.*}": expectRequestBody(t, map[string]any{ "message": "Create new file", "content": "IyBOZXcgRmlsZQoKQ3JlYXRlZCB3aXRob3V0IFNIQQ==", diff --git a/pkg/utils/api.go b/pkg/utils/api.go index a22711b23..ae3a9afc3 100644 --- a/pkg/utils/api.go +++ b/pkg/utils/api.go @@ -235,11 +235,11 @@ func parseAPIHost(s string) (APIHost, error) { return APIHost{}, fmt.Errorf("host must have a scheme (http or https): %s", s) } - if strings.HasSuffix(u.Hostname(), "github.com") { + if u.Hostname() == "github.com" || strings.HasSuffix(u.Hostname(), ".github.com") { return newDotcomHost() } - if strings.HasSuffix(u.Hostname(), "ghe.com") { + if u.Hostname() == "ghe.com" || strings.HasSuffix(u.Hostname(), ".ghe.com") { return newGHECHost(s) } diff --git a/pkg/utils/api_test.go b/pkg/utils/api_test.go new file mode 100644 index 000000000..40fcb8f26 --- /dev/null +++ b/pkg/utils/api_test.go @@ -0,0 +1,75 @@ +package utils //nolint:revive //TODO: figure out a better name for this package + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseAPIHost(t *testing.T) { + tests := []struct { + name string + input string + wantRestURL string + wantErr bool + }{ + { + name: "empty string defaults to dotcom", + input: "", + wantRestURL: "https://api.github.com/", + }, + { + name: "github.com hostname", + input: "https://github.com", + wantRestURL: "https://api.github.com/", + }, + { + name: "subdomain of github.com", + input: "https://foo.github.com", + wantRestURL: "https://api.github.com/", + }, + { + name: "hostname ending in github.com but not a subdomain", + input: "https://mycompanygithub.com", + wantRestURL: "https://mycompanygithub.com/api/v3/", + }, + { + name: "hostname ending in notgithub.com", + input: "https://notgithub.com", + wantRestURL: "https://notgithub.com/api/v3/", + }, + { + name: "ghe.com hostname", + input: "https://ghe.com", + wantRestURL: "https://api.ghe.com/", + }, + { + name: "subdomain of ghe.com", + input: "https://mycompany.ghe.com", + wantRestURL: "https://api.mycompany.ghe.com/", + }, + { + name: "hostname ending in ghe.com but not a subdomain", + input: "https://myghe.com", + wantRestURL: "https://myghe.com/api/v3/", + }, + { + name: "missing scheme", + input: "github.com", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + host, err := parseAPIHost(tc.input) + if tc.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.wantRestURL, host.restURL.String()) + }) + } +}