Skip to content

Use arrays rather than CSV #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
- `repo`: Repository name (string, required)
- `title`: Issue title (string, required)
- `body`: Issue body content (string, optional)
- `assignees`: Comma-separated list of usernames to assign to this issue (string, optional)
- `labels`: Comma-separated list of labels to apply to this issue (string, optional)
- `assignees`: Usernames to assign to this issue (string[], optional)
- `labels`: Labels to apply to this issue (string[], optional)

- **add_issue_comment** - Add a comment to an issue

Expand All @@ -145,7 +145,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
- `owner`: Repository owner (string, required)
- `repo`: Repository name (string, required)
- `state`: Filter by state ('open', 'closed', 'all') (string, optional)
- `labels`: Comma-separated list of labels to filter by (string, optional)
- `labels`: Labels to filter by (string[], optional)
- `sort`: Sort by ('created', 'updated', 'comments') (string, optional)
- `direction`: Sort direction ('asc', 'desc') (string, optional)
- `since`: Filter by date (ISO 8601 timestamp) (string, optional)
Expand All @@ -160,8 +160,8 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
- `title`: New title (string, optional)
- `body`: New description (string, optional)
- `state`: New state ('open' or 'closed') (string, optional)
- `labels`: Comma-separated list of new labels (string, optional)
- `assignees`: Comma-separated list of new assignees (string, optional)
- `labels`: New labels (string[], optional)
- `assignees`: New assignees (string[], optional)
- `milestone`: New milestone number (number, optional)

- **search_issues** - Search for issues and pull requests
Expand Down
46 changes: 46 additions & 0 deletions pkg/github/helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,52 @@ import (
"github.com/stretchr/testify/require"
)

// expectQueryParams is a helper function to create a partial mock that expects a
// request with the given query parameters, with the ability to chain a response handler.
func expectQueryParams(t *testing.T, expectedQueryParams map[string]string) *partialMock {
return &partialMock{
t: t,
expectedQueryParams: expectedQueryParams,
}
}

// expectRequestBody is a helper function to create a partial mock that expects a
// request with the given body, with the ability to chain a response handler.
func expectRequestBody(t *testing.T, expectedRequestBody any) *partialMock {
return &partialMock{
t: t,
expectedRequestBody: expectedRequestBody,
}
}

type partialMock struct {
t *testing.T
expectedQueryParams map[string]string
expectedRequestBody any
}

func (p *partialMock) andThen(responseHandler http.HandlerFunc) http.HandlerFunc {
p.t.Helper()
return func(w http.ResponseWriter, r *http.Request) {
if p.expectedRequestBody != nil {
var unmarshaledRequestBody any
err := json.NewDecoder(r.Body).Decode(&unmarshaledRequestBody)
require.NoError(p.t, err)

require.Equal(p.t, p.expectedRequestBody, unmarshaledRequestBody)
}

if p.expectedQueryParams != nil {
require.Equal(p.t, len(p.expectedQueryParams), len(r.URL.Query()))
for k, v := range p.expectedQueryParams {
require.Equal(p.t, v, r.URL.Query().Get(k))
}
}

responseHandler(w, r)
}
}

// mockResponse is a helper function to create a mock HTTP response handler
// that returns a specified status code and marshaled body.
func mockResponse(t *testing.T, code int, body interface{}) http.HandlerFunc {
Expand Down
66 changes: 48 additions & 18 deletions pkg/github/issues.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,11 +228,21 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
mcp.WithString("body",
mcp.Description("Issue body content"),
),
mcp.WithString("assignees",
mcp.Description("Comma-separate list of usernames to assign to this issue"),
),
mcp.WithString("labels",
mcp.Description("Comma-separate list of labels to apply to this issue"),
mcp.WithArray("assignees",
mcp.Description("Usernames to assign to this issue"),
mcp.Items(
map[string]interface{}{
"type": "string",
},
),
),
mcp.WithArray("labels",
mcp.Description("Labels to apply to this issue"),
mcp.Items(
map[string]interface{}{
"type": "string",
},
),
),
),
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
Expand All @@ -256,12 +266,13 @@ func createIssue(client *github.Client, t translations.TranslationHelperFunc) (t
}

// Get assignees
assignees, err := optionalCommaSeparatedListParam(request, "assignees")
assignees, err := optionalParam[[]string](request, "assignees")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}

// Get labels
labels, err := optionalCommaSeparatedListParam(request, "labels")
labels, err := optionalParam[[]string](request, "labels")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
Expand Down Expand Up @@ -312,8 +323,13 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
mcp.WithString("state",
mcp.Description("Filter by state ('open', 'closed', 'all')"),
),
mcp.WithString("labels",
mcp.Description("Comma-separated list of labels to filter by"),
mcp.WithArray("labels",
mcp.Description("Filter by labels"),
mcp.Items(
map[string]interface{}{
"type": "string",
},
),
),
mcp.WithString("sort",
mcp.Description("Sort by ('created', 'updated', 'comments')"),
Expand Down Expand Up @@ -349,7 +365,8 @@ func listIssues(client *github.Client, t translations.TranslationHelperFunc) (to
return mcp.NewToolResultError(err.Error()), nil
}

opts.Labels, err = optionalCommaSeparatedListParam(request, "labels")
// Get labels
opts.Labels, err = optionalParam[[]string](request, "labels")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
Expand Down Expand Up @@ -431,12 +448,23 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
),
mcp.WithString("state",
mcp.Description("New state ('open' or 'closed')"),
),
mcp.WithString("labels",
mcp.Description("Comma-separated list of new labels"),
),
mcp.WithString("assignees",
mcp.Description("Comma-separated list of new assignees"),
mcp.Enum("open", "closed"),
),
mcp.WithArray("labels",
mcp.Description("New labels"),
mcp.Items(
map[string]interface{}{
"type": "string",
},
),
),
mcp.WithArray("assignees",
mcp.Description("New assignees"),
mcp.Items(
map[string]interface{}{
"type": "string",
},
),
),
mcp.WithNumber("milestone",
mcp.Description("New milestone number"),
Expand Down Expand Up @@ -484,15 +512,17 @@ func updateIssue(client *github.Client, t translations.TranslationHelperFunc) (t
issueRequest.State = github.Ptr(state)
}

labels, err := optionalCommaSeparatedListParam(request, "labels")
// Get labels
labels, err := optionalParam[[]string](request, "labels")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
if len(labels) > 0 {
issueRequest.Labels = &labels
}

assignees, err := optionalCommaSeparatedListParam(request, "assignees")
// Get assignees
assignees, err := optionalParam[[]string](request, "assignees")
if err != nil {
return mcp.NewToolResultError(err.Error()), nil
}
Expand Down
44 changes: 35 additions & 9 deletions pkg/github/issues_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,16 +418,23 @@ func Test_CreateIssue(t *testing.T) {
mockedClient: mock.NewMockedHTTPClient(
mock.WithRequestMatchHandler(
mock.PostReposIssuesByOwnerByRepo,
mockResponse(t, http.StatusCreated, mockIssue),
expectRequestBody(t, map[string]any{
"title": "Test Issue",
"body": "This is a test issue",
"labels": []any{"bug", "help wanted"},
"assignees": []any{"user1", "user2"},
}).andThen(
mockResponse(t, http.StatusCreated, mockIssue),
),
),
),
requestArgs: map[string]interface{}{
"owner": "owner",
"repo": "repo",
"title": "Test Issue",
"body": "This is a test issue",
"assignees": "user1, user2",
"labels": "bug, help wanted",
"assignees": []string{"user1", "user2"},
"labels": []string{"bug", "help wanted"},
},
expectError: false,
expectedIssue: mockIssue,
Expand Down Expand Up @@ -606,16 +613,26 @@ func Test_ListIssues(t *testing.T) {
{
name: "list issues with all parameters",
mockedClient: mock.NewMockedHTTPClient(
mock.WithRequestMatch(
mock.WithRequestMatchHandler(
mock.GetReposIssuesByOwnerByRepo,
mockIssues,
expectQueryParams(t, map[string]string{
"state": "open",
"labels": "bug,enhancement",
"sort": "created",
"direction": "desc",
"since": "2023-01-01T00:00:00Z",
"page": "1",
"per_page": "30",
}).andThen(
mockResponse(t, http.StatusOK, mockIssues),
),
),
),
requestArgs: map[string]interface{}{
"owner": "owner",
"repo": "repo",
"state": "open",
"labels": "bug,enhancement",
"labels": []string{"bug", "enhancement"},
"sort": "created",
"direction": "desc",
"since": "2023-01-01T00:00:00Z",
Expand Down Expand Up @@ -750,7 +767,16 @@ func Test_UpdateIssue(t *testing.T) {
mockedClient: mock.NewMockedHTTPClient(
mock.WithRequestMatchHandler(
mock.PatchReposIssuesByOwnerByRepoByIssueNumber,
mockResponse(t, http.StatusOK, mockIssue),
expectRequestBody(t, map[string]any{
"title": "Updated Issue Title",
"body": "Updated issue description",
"state": "closed",
"labels": []any{"bug", "priority"},
"assignees": []any{"assignee1", "assignee2"},
"milestone": float64(5),
}).andThen(
mockResponse(t, http.StatusOK, mockIssue),
),
),
),
requestArgs: map[string]interface{}{
Expand All @@ -760,8 +786,8 @@ func Test_UpdateIssue(t *testing.T) {
"title": "Updated Issue Title",
"body": "Updated issue description",
"state": "closed",
"labels": "bug,priority",
"assignees": "assignee1,assignee2",
"labels": []string{"bug", "priority"},
"assignees": []string{"assignee1", "assignee2"},
"milestone": float64(5),
},
expectError: false,
Expand Down
37 changes: 0 additions & 37 deletions pkg/github/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"io"
"net/http"
"strings"

"github.com/github/github-mcp-server/pkg/translations"
"github.com/google/go-github/v69/github"
Expand Down Expand Up @@ -119,25 +118,6 @@ func isAcceptedError(err error) bool {
return errors.As(err, &acceptedError)
}

// parseCommaSeparatedList is a helper function that parses a comma-separated list of strings from the input string.
func parseCommaSeparatedList(input string) []string {
if input == "" {
return nil
}

parts := strings.Split(input, ",")
result := make([]string, 0, len(parts))

for _, part := range parts {
trimmed := strings.TrimSpace(part)
if trimmed != "" {
result = append(result, trimmed)
}
}

return result
}

// 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.
Expand Down Expand Up @@ -221,20 +201,3 @@ func optionalIntParamWithDefault(r mcp.CallToolRequest, p string, d int) (int, e
}
return v, nil
}

// optionalCommaSeparatedListParam is a helper function that can be used to fetch a requested parameter from the request.
// It does the following:
// 1. Checks if the parameter is present in the request, if not, it returns an empty list
// 2. If it is present, it checks if the parameter is of the expected type and uses parseCommaSeparatedList to parse it
// and return the list of strings
func optionalCommaSeparatedListParam(r mcp.CallToolRequest, p string) ([]string, error) {
v, err := optionalParam[string](r, p)
if err != nil {
return []string{}, err
}
l := parseCommaSeparatedList(v)
if len(l) == 0 {
return []string{}, nil
}
return l, nil
}
Loading