From 65a45588a904aed342e336dc72fe25d5a1490f6e Mon Sep 17 00:00:00 2001 From: Tom X Nguyen Date: Sat, 5 Apr 2025 14:59:56 +0700 Subject: [PATCH 1/5] feat: add update_pull_request tool --- pkg/github/pullrequests.go | 113 ++++++++++++++++++++ pkg/github/pullrequests_test.go | 182 ++++++++++++++++++++++++++++++++ pkg/github/server.go | 21 ++++ 3 files changed, 316 insertions(+) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index c02336ca..d61cdf28 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -67,6 +67,119 @@ func getPullRequest(client *github.Client, t translations.TranslationHelperFunc) } } +// updatePullRequest creates a tool to update an existing pull request. +func updatePullRequest(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { + return mcp.NewTool("update_pull_request", + mcp.WithDescription(t("TOOL_UPDATE_PULL_REQUEST_DESCRIPTION", "Update an existing pull request in a GitHub repository")), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("Repository owner"), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("Repository name"), + ), + mcp.WithNumber("pullNumber", + mcp.Required(), + mcp.Description("Pull request number to update"), + ), + mcp.WithString("title", + mcp.Description("New title"), + ), + mcp.WithString("body", + mcp.Description("New description"), + ), + mcp.WithString("state", + mcp.Description("New state ('open' or 'closed')"), + mcp.Enum("open", "closed"), + ), + mcp.WithString("base", + mcp.Description("New base branch name"), + ), + mcp.WithBoolean("maintainer_can_modify", + mcp.Description("Allow maintainer edits"), + ), + ), + 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 + } + + // Build the update struct only with provided fields + update := &github.PullRequest{} + updateNeeded := false + + if title, ok, err := optionalParamOk[string](request, "title"); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } else if ok { + update.Title = github.Ptr(title) + updateNeeded = true + } + + if body, ok, err := optionalParamOk[string](request, "body"); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } else if ok { + update.Body = github.Ptr(body) + updateNeeded = true + } + + if state, ok, err := optionalParamOk[string](request, "state"); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } else if ok { + update.State = github.Ptr(state) + updateNeeded = true + } + + if base, ok, err := optionalParamOk[string](request, "base"); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } else if ok { + update.Base = &github.PullRequestBranch{Ref: github.Ptr(base)} + updateNeeded = true + } + + if maintainerCanModify, ok, err := optionalParamOk[bool](request, "maintainer_can_modify"); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } else if ok { + update.MaintainerCanModify = github.Ptr(maintainerCanModify) + updateNeeded = true + } + + if !updateNeeded { + return mcp.NewToolResultError("No update parameters provided."), nil + } + + pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update) + if err != nil { + return nil, fmt.Errorf("failed to update pull request: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + 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 mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil + } + + r, err := json.Marshal(pr) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return mcp.NewToolResultText(string(r)), nil + } +} + // listPullRequests creates a tool to list and filter repository pull requests. func listPullRequests(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("list_pull_requests", diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index b666e8a8..c9cf39b7 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -126,6 +126,188 @@ func Test_GetPullRequest(t *testing.T) { } } +func Test_UpdatePullRequest(t *testing.T) { + // Verify tool definition once + mockClient := github.NewClient(nil) + tool, _ := updatePullRequest(mockClient, translations.NullTranslationHelper) + + assert.Equal(t, "update_pull_request", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "pullNumber") + assert.Contains(t, tool.InputSchema.Properties, "title") + assert.Contains(t, tool.InputSchema.Properties, "body") + assert.Contains(t, tool.InputSchema.Properties, "state") + assert.Contains(t, tool.InputSchema.Properties, "base") + assert.Contains(t, tool.InputSchema.Properties, "maintainer_can_modify") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"}) + + // Setup mock PR for success case + mockUpdatedPR := &github.PullRequest{ + Number: github.Ptr(42), + Title: github.Ptr("Updated Test PR Title"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42"), + Body: github.Ptr("Updated test PR body."), + MaintainerCanModify: github.Ptr(false), + Base: &github.PullRequestBranch{ + Ref: github.Ptr("develop"), + }, + } + + mockClosedPR := &github.PullRequest{ + Number: github.Ptr(42), + Title: github.Ptr("Test PR"), + State: github.Ptr("closed"), // State updated + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedPR *github.PullRequest + expectedErrMsg string + }{ + { + name: "successful PR update (title, body, base, maintainer_can_modify)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposPullsByOwnerByRepoByPullNumber, + // Expect the flat string based on previous test failure output and API docs + expectRequestBody(t, map[string]interface{}{ + "title": "Updated Test PR Title", + "body": "Updated test PR body.", + "base": "develop", + "maintainer_can_modify": false, + }).andThen( + mockResponse(t, http.StatusOK, mockUpdatedPR), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "title": "Updated Test PR Title", + "body": "Updated test PR body.", + "base": "develop", + "maintainer_can_modify": false, + }, + expectError: false, + expectedPR: mockUpdatedPR, + }, + { + name: "successful PR update (state)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposPullsByOwnerByRepoByPullNumber, + expectRequestBody(t, map[string]interface{}{ + "state": "closed", + }).andThen( + mockResponse(t, http.StatusOK, mockClosedPR), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "state": "closed", + }, + expectError: false, + expectedPR: mockClosedPR, + }, + { + name: "no update parameters provided", + mockedClient: mock.NewMockedHTTPClient(), // No API call expected + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + // No update fields + }, + expectError: false, // Error is returned in the result, not as Go error + expectedErrMsg: "No update parameters provided", + }, + { + name: "PR update fails (API error)", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposPullsByOwnerByRepoByPullNumber, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "title": "Invalid Title Causing Error", + }, + expectError: true, + expectedErrMsg: "failed to update pull request", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + _, handler := updatePullRequest(client, translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + + // Verify results + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + + require.NoError(t, err) + + // Parse the result and get the text content + textContent := getTextResult(t, result) + + // Check for expected error message within the result text + if tc.expectedErrMsg != "" { + assert.Contains(t, textContent.Text, tc.expectedErrMsg) + return + } + + // Unmarshal and verify the successful result + var returnedPR github.PullRequest + err = json.Unmarshal([]byte(textContent.Text), &returnedPR) + require.NoError(t, err) + assert.Equal(t, *tc.expectedPR.Number, *returnedPR.Number) + if tc.expectedPR.Title != nil { + assert.Equal(t, *tc.expectedPR.Title, *returnedPR.Title) + } + if tc.expectedPR.Body != nil { + assert.Equal(t, *tc.expectedPR.Body, *returnedPR.Body) + } + if tc.expectedPR.State != nil { + assert.Equal(t, *tc.expectedPR.State, *returnedPR.State) + } + if tc.expectedPR.Base != nil && tc.expectedPR.Base.Ref != nil { + assert.NotNil(t, returnedPR.Base) + assert.Equal(t, *tc.expectedPR.Base.Ref, *returnedPR.Base.Ref) + } + if tc.expectedPR.MaintainerCanModify != nil { + assert.Equal(t, *tc.expectedPR.MaintainerCanModify, *returnedPR.MaintainerCanModify) + } + }) + } +} + func Test_ListPullRequests(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) diff --git a/pkg/github/server.go b/pkg/github/server.go index 66dbfd1c..671dcf82 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -53,6 +53,7 @@ func NewServer(client *github.Client, readOnly bool, t translations.TranslationH s.AddTool(updatePullRequestBranch(client, t)) s.AddTool(createPullRequestReview(client, t)) s.AddTool(createPullRequest(client, t)) + s.AddTool(updatePullRequest(client, t)) } // Add GitHub tools - Repositories @@ -112,6 +113,26 @@ func getMe(client *github.Client, t translations.TranslationHelperFunc) (tool mc } } +// optionalParamOk is a helper function that can be used to fetch a requested parameter from the request. +// It returns the value, a boolean indicating if the parameter was present, and an error if the type is wrong. +func optionalParamOk[T any](r mcp.CallToolRequest, p string) (T, bool, error) { + var zero T + + // Check if the parameter is present in the request + val, ok := r.Params.Arguments[p] + if !ok { + return zero, false, nil // Not present, return zero value, false, no error + } + + // Check if the parameter is of the expected type + typedVal, ok := val.(T) + if !ok { + return zero, true, fmt.Errorf("parameter %s is not of type %T, is %T", p, zero, val) // Present but wrong type + } + + return typedVal, true, nil // Present and correct type +} + // isAcceptedError checks if the error is an accepted error. func isAcceptedError(err error) bool { var acceptedError *github.AcceptedError From d206a7c2e81979a565c2b975566324d67f203bf2 Mon Sep 17 00:00:00 2001 From: Tom X Nguyen Date: Sat, 5 Apr 2025 15:40:25 +0700 Subject: [PATCH 2/5] refactor: address feedback on optionalParamOK helper --- pkg/github/helper_test.go | 112 +++++++++++++++++++++++++++++++++++++ pkg/github/pullrequests.go | 10 ++-- pkg/github/server.go | 24 ++++---- 3 files changed, 131 insertions(+), 15 deletions(-) diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 9dcffa42..98957638 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -93,3 +93,115 @@ func getTextResult(t *testing.T, result *mcp.CallToolResult) mcp.TextContent { assert.Equal(t, "text", textContent.Type) return textContent } + +func TestOptionalParamOK(t *testing.T) { + tests := []struct { + name string + args map[string]interface{} + paramName string + expectedVal interface{} + expectedOk bool + expectError bool + errorMsg string + }{ + { + name: "present and correct type (string)", + args: map[string]interface{}{"myParam": "hello"}, + paramName: "myParam", + expectedVal: "hello", + expectedOk: true, + expectError: false, + }, + { + name: "present and correct type (bool)", + args: map[string]interface{}{"myParam": true}, + paramName: "myParam", + expectedVal: true, + expectedOk: true, + expectError: false, + }, + { + name: "present and correct type (number)", + args: map[string]interface{}{"myParam": float64(123)}, + paramName: "myParam", + expectedVal: float64(123), + expectedOk: true, + expectError: false, + }, + { + name: "present but wrong type (string expected, got bool)", + args: map[string]interface{}{"myParam": true}, + paramName: "myParam", + expectedVal: "", // Zero value for string + expectedOk: true, // ok is true because param exists + expectError: true, + errorMsg: "parameter myParam is not of type string, is bool", + }, + { + name: "present but wrong type (bool expected, got string)", + args: map[string]interface{}{"myParam": "true"}, + paramName: "myParam", + expectedVal: false, // Zero value for bool + expectedOk: true, // ok is true because param exists + expectError: true, + errorMsg: "parameter myParam is not of type bool, is string", + }, + { + name: "parameter not present", + args: map[string]interface{}{"anotherParam": "value"}, + paramName: "myParam", + expectedVal: "", // Zero value for string + expectedOk: false, + expectError: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + request := createMCPRequest(tc.args) + + // Test with string type assertion + if _, isString := tc.expectedVal.(string); isString || tc.errorMsg == "parameter myParam is not of type string, is bool" { + val, ok, err := optionalParamOK[string](request, tc.paramName) + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorMsg) + assert.Equal(t, tc.expectedOk, ok) // Check ok even on error + assert.Equal(t, tc.expectedVal, val) // Check zero value on error + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedOk, ok) + assert.Equal(t, tc.expectedVal, val) + } + } + + // Test with bool type assertion + if _, isBool := tc.expectedVal.(bool); isBool || tc.errorMsg == "parameter myParam is not of type bool, is string" { + val, ok, err := optionalParamOK[bool](request, tc.paramName) + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorMsg) + assert.Equal(t, tc.expectedOk, ok) // Check ok even on error + assert.Equal(t, tc.expectedVal, val) // Check zero value on error + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedOk, ok) + assert.Equal(t, tc.expectedVal, val) + } + } + + // Test with float64 type assertion (for number case) + if _, isFloat := tc.expectedVal.(float64); isFloat { + val, ok, err := optionalParamOK[float64](request, tc.paramName) + if tc.expectError { + // This case shouldn't happen for float64 in the defined tests + require.Fail(t, "Unexpected error case for float64") + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedOk, ok) + assert.Equal(t, tc.expectedVal, val) + } + } + }) + } +} diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index d61cdf28..a0854514 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -118,35 +118,35 @@ func updatePullRequest(client *github.Client, t translations.TranslationHelperFu update := &github.PullRequest{} updateNeeded := false - if title, ok, err := optionalParamOk[string](request, "title"); err != nil { + if title, ok, err := optionalParamOK[string](request, "title"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Title = github.Ptr(title) updateNeeded = true } - if body, ok, err := optionalParamOk[string](request, "body"); err != nil { + if body, ok, err := optionalParamOK[string](request, "body"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Body = github.Ptr(body) updateNeeded = true } - if state, ok, err := optionalParamOk[string](request, "state"); err != nil { + if state, ok, err := optionalParamOK[string](request, "state"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.State = github.Ptr(state) updateNeeded = true } - if base, ok, err := optionalParamOk[string](request, "base"); err != nil { + if base, ok, err := optionalParamOK[string](request, "base"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Base = &github.PullRequestBranch{Ref: github.Ptr(base)} updateNeeded = true } - if maintainerCanModify, ok, err := optionalParamOk[bool](request, "maintainer_can_modify"); err != nil { + if maintainerCanModify, ok, err := optionalParamOK[bool](request, "maintainer_can_modify"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.MaintainerCanModify = github.Ptr(maintainerCanModify) diff --git a/pkg/github/server.go b/pkg/github/server.go index 671dcf82..07aa1222 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -113,24 +113,28 @@ func getMe(client *github.Client, t translations.TranslationHelperFunc) (tool mc } } -// optionalParamOk is a helper function that can be used to fetch a requested parameter from the request. +// optionalParamOK is a helper function that can be used to fetch a requested parameter from the request. // It returns the value, a boolean indicating if the parameter was present, and an error if the type is wrong. -func optionalParamOk[T any](r mcp.CallToolRequest, p string) (T, bool, error) { - var zero T - +func optionalParamOK[T any](r mcp.CallToolRequest, p string) (value T, ok bool, err error) { // Check if the parameter is present in the request - val, ok := r.Params.Arguments[p] - if !ok { - return zero, false, nil // Not present, return zero value, false, no error + val, exists := r.Params.Arguments[p] + if !exists { + // Not present, return zero value, false, no error + return } // Check if the parameter is of the expected type - typedVal, ok := val.(T) + value, ok = val.(T) if !ok { - return zero, true, fmt.Errorf("parameter %s is not of type %T, is %T", p, zero, val) // Present but wrong type + // Present but wrong type + err = fmt.Errorf("parameter %s is not of type %T, is %T", p, value, val) + ok = true // Set ok to true because the parameter *was* present, even if wrong type + return } - return typedVal, true, nil // Present and correct type + // Present and correct type + ok = true + return } // isAcceptedError checks if the error is an accepted error. From 88f56a7aef9723b0bc617a62829dd4fcbb9aa878 Mon Sep 17 00:00:00 2001 From: Tom X Nguyen Date: Sun, 6 Apr 2025 00:05:46 +0700 Subject: [PATCH 3/5] docs: add update_pull_request tool documentation --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 9330723c..c91c2168 100644 --- a/README.md +++ b/README.md @@ -279,6 +279,17 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `draft`: Create as draft PR (boolean, optional) - `maintainer_can_modify`: Allow maintainer edits (boolean, optional) +- **update_pull_request** - Update an existing pull request in a GitHub repository + + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - `pullNumber`: Pull request number to update (number, required) + - `title`: New title (string, optional) + - `body`: New description (string, optional) + - `state`: New state ('open' or 'closed') (string, optional) + - `base`: New base branch name (string, optional) + - `maintainer_can_modify`: Allow maintainer edits (boolean, optional) + ### Repositories - **create_or_update_file** - Create or update a single file in a repository From 57d2a4281b60901b08b1f32e516cf3ee66037712 Mon Sep 17 00:00:00 2001 From: Tom X Nguyen Date: Tue, 8 Apr 2025 15:03:31 +0700 Subject: [PATCH 4/5] refactor: update optionalParamsOK as exported member --- pkg/github/helper_test.go | 6 +++--- pkg/github/pullrequests.go | 10 +++++----- pkg/github/server.go | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 98957638..40fc0b94 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -162,7 +162,7 @@ func TestOptionalParamOK(t *testing.T) { // Test with string type assertion if _, isString := tc.expectedVal.(string); isString || tc.errorMsg == "parameter myParam is not of type string, is bool" { - val, ok, err := optionalParamOK[string](request, tc.paramName) + val, ok, err := OptionalParamOK[string](request, tc.paramName) if tc.expectError { require.Error(t, err) assert.Contains(t, err.Error(), tc.errorMsg) @@ -177,7 +177,7 @@ func TestOptionalParamOK(t *testing.T) { // Test with bool type assertion if _, isBool := tc.expectedVal.(bool); isBool || tc.errorMsg == "parameter myParam is not of type bool, is string" { - val, ok, err := optionalParamOK[bool](request, tc.paramName) + val, ok, err := OptionalParamOK[bool](request, tc.paramName) if tc.expectError { require.Error(t, err) assert.Contains(t, err.Error(), tc.errorMsg) @@ -192,7 +192,7 @@ func TestOptionalParamOK(t *testing.T) { // Test with float64 type assertion (for number case) if _, isFloat := tc.expectedVal.(float64); isFloat { - val, ok, err := optionalParamOK[float64](request, tc.paramName) + val, ok, err := OptionalParamOK[float64](request, tc.paramName) if tc.expectError { // This case shouldn't happen for float64 in the defined tests require.Fail(t, "Unexpected error case for float64") diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index ed236955..c5f9d9fa 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -118,35 +118,35 @@ func UpdatePullRequest(client *github.Client, t translations.TranslationHelperFu update := &github.PullRequest{} updateNeeded := false - if title, ok, err := optionalParamOK[string](request, "title"); err != nil { + if title, ok, err := OptionalParamOK[string](request, "title"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Title = github.Ptr(title) updateNeeded = true } - if body, ok, err := optionalParamOK[string](request, "body"); err != nil { + if body, ok, err := OptionalParamOK[string](request, "body"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Body = github.Ptr(body) updateNeeded = true } - if state, ok, err := optionalParamOK[string](request, "state"); err != nil { + if state, ok, err := OptionalParamOK[string](request, "state"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.State = github.Ptr(state) updateNeeded = true } - if base, ok, err := optionalParamOK[string](request, "base"); err != nil { + if base, ok, err := OptionalParamOK[string](request, "base"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.Base = &github.PullRequestBranch{Ref: github.Ptr(base)} updateNeeded = true } - if maintainerCanModify, ok, err := optionalParamOK[bool](request, "maintainer_can_modify"); err != nil { + if maintainerCanModify, ok, err := OptionalParamOK[bool](request, "maintainer_can_modify"); err != nil { return mcp.NewToolResultError(err.Error()), nil } else if ok { update.MaintainerCanModify = github.Ptr(maintainerCanModify) diff --git a/pkg/github/server.go b/pkg/github/server.go index 486001d6..84c15f50 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -113,9 +113,9 @@ func GetMe(client *github.Client, t translations.TranslationHelperFunc) (tool mc } } -// optionalParamOK is a helper function that can be used to fetch a requested parameter from the request. +// OptionalParamOK is a helper function that can be used to fetch a requested parameter from the request. // It returns the value, a boolean indicating if the parameter was present, and an error if the type is wrong. -func optionalParamOK[T any](r mcp.CallToolRequest, p string) (value T, ok bool, err error) { +func OptionalParamOK[T any](r mcp.CallToolRequest, p string) (value T, ok bool, err error) { // Check if the parameter is present in the request val, exists := r.Params.Arguments[p] if !exists { From 9a46dbd2bb5741ac2c9705d84bc24ee725b7cbff Mon Sep 17 00:00:00 2001 From: Tom X Nguyen Date: Tue, 8 Apr 2025 15:12:36 +0700 Subject: [PATCH 5/5] fix: rename to exported function --- pkg/github/pullrequests_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 69c0b407..e9647029 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -129,7 +129,7 @@ func Test_GetPullRequest(t *testing.T) { func Test_UpdatePullRequest(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := updatePullRequest(mockClient, translations.NullTranslationHelper) + tool, _ := UpdatePullRequest(mockClient, translations.NullTranslationHelper) assert.Equal(t, "update_pull_request", tool.Name) assert.NotEmpty(t, tool.Description) @@ -257,7 +257,7 @@ func Test_UpdatePullRequest(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := updatePullRequest(client, translations.NullTranslationHelper) + _, handler := UpdatePullRequest(client, translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs)