Skip to content

Commit d9292e9

Browse files
mattdhollowayCopilot
mattdholloway
and
Copilot
committed
Address ui_get review feedback
- Paginate the labels GraphQL query (cursor-based) so repos with more than 100 labels return a complete list instead of silently truncating. - Emit an empty due_on for milestones without a due date instead of formatting the zero time as "0001-01-01". - Use NewGitHubAPIErrorResponse in uiGetIssueTypes to preserve GitHub response context, matching the other REST-backed methods. - Extend tests to cover the labels (GraphQL), milestones (including the no-due-date case) and issue_types methods, plus the issue_types error path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent f310291 commit d9292e9

2 files changed

Lines changed: 185 additions & 35 deletions

File tree

‎pkg/github/ui_tools.go‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -109,32 +109,44 @@ func uiGetLabels(ctx context.Context, deps ToolDependencies, args map[string]any
109109
Description githubv4.String
110110
}
111111
TotalCount githubv4.Int
112-
} `graphql:"labels(first: 100)"`
112+
PageInfo struct {
113+
HasNextPage githubv4.Boolean
114+
EndCursor githubv4.String
115+
}
116+
} `graphql:"labels(first: 100, after: $cursor)"`
113117
} `graphql:"repository(owner: $owner, name: $repo)"`
114118
}
115119

116120
vars := map[string]any{
117-
"owner": githubv4.String(owner),
118-
"repo": githubv4.String(repo),
119-
}
120-
121-
if err := client.Query(ctx, &query, vars); err != nil {
122-
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to list labels", err), nil, nil
121+
"owner": githubv4.String(owner),
122+
"repo": githubv4.String(repo),
123+
"cursor": (*githubv4.String)(nil),
123124
}
124125

125-
labels := make([]map[string]any, len(query.Repository.Labels.Nodes))
126-
for i, labelNode := range query.Repository.Labels.Nodes {
127-
labels[i] = map[string]any{
128-
"id": fmt.Sprintf("%v", labelNode.ID),
129-
"name": string(labelNode.Name),
130-
"color": string(labelNode.Color),
131-
"description": string(labelNode.Description),
126+
labels := make([]map[string]any, 0)
127+
var totalCount int
128+
for {
129+
if err := client.Query(ctx, &query, vars); err != nil {
130+
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to list labels", err), nil, nil
132131
}
132+
for _, labelNode := range query.Repository.Labels.Nodes {
133+
labels = append(labels, map[string]any{
134+
"id": fmt.Sprintf("%v", labelNode.ID),
135+
"name": string(labelNode.Name),
136+
"color": string(labelNode.Color),
137+
"description": string(labelNode.Description),
138+
})
139+
}
140+
totalCount = int(query.Repository.Labels.TotalCount)
141+
if !query.Repository.Labels.PageInfo.HasNextPage {
142+
break
143+
}
144+
vars["cursor"] = githubv4.NewString(query.Repository.Labels.PageInfo.EndCursor)
133145
}
134146

135147
response := map[string]any{
136148
"labels": labels,
137-
"totalCount": int(query.Repository.Labels.TotalCount),
149+
"totalCount": totalCount,
138150
}
139151

140152
out, err := json.Marshal(response)
@@ -221,13 +233,17 @@ func uiGetMilestones(ctx context.Context, deps ToolDependencies, args map[string
221233

222234
result := make([]map[string]any, len(allMilestones))
223235
for i, m := range allMilestones {
236+
dueOn := ""
237+
if m.DueOn != nil {
238+
dueOn = m.GetDueOn().Format("2006-01-02")
239+
}
224240
result[i] = map[string]any{
225241
"number": m.GetNumber(),
226242
"title": m.GetTitle(),
227243
"description": m.GetDescription(),
228244
"state": m.GetState(),
229245
"open_issues": m.GetOpenIssues(),
230-
"due_on": m.GetDueOn().Format("2006-01-02"),
246+
"due_on": dueOn,
231247
}
232248
}
233249

@@ -250,7 +266,7 @@ func uiGetIssueTypes(ctx context.Context, deps ToolDependencies, owner string) (
250266

251267
issueTypes, resp, err := client.Organizations.ListIssueTypes(ctx, owner)
252268
if err != nil {
253-
return utils.NewToolResultErrorFromErr("failed to list issue types", err), nil, nil
269+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list issue types", resp, err), nil, nil
254270
}
255271
defer func() { _ = resp.Body.Close() }()
256272

‎pkg/github/ui_tools_test.go‎

Lines changed: 152 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,14 @@ import (
55
"encoding/json"
66
"net/http"
77
"testing"
8+
"time"
89

10+
"github.com/github/github-mcp-server/internal/githubv4mock"
911
"github.com/github/github-mcp-server/internal/toolsnaps"
1012
"github.com/github/github-mcp-server/pkg/translations"
1113
"github.com/google/go-github/v87/github"
1214
"github.com/google/jsonschema-go/jsonschema"
15+
"github.com/shurcooL/githubv4"
1316
"github.com/stretchr/testify/assert"
1417
"github.com/stretchr/testify/require"
1518
)
@@ -46,13 +49,25 @@ func Test_UIGet(t *testing.T) {
4649
{Name: github.Ptr("feature"), Protected: github.Ptr(false)},
4750
}
4851

52+
dueDate := time.Date(2026, 1, 31, 0, 0, 0, 0, time.UTC)
53+
mockMilestones := []*github.Milestone{
54+
{Number: github.Ptr(1), Title: github.Ptr("with due date"), DueOn: &github.Timestamp{Time: dueDate}},
55+
{Number: github.Ptr(2), Title: github.Ptr("no due date")},
56+
}
57+
58+
mockIssueTypes := []*github.IssueType{
59+
{Name: github.Ptr("Bug")},
60+
{Name: github.Ptr("Feature")},
61+
}
62+
4963
tests := []struct {
50-
name string
51-
mockedClient *http.Client
52-
requestArgs map[string]any
53-
expectError bool
54-
expectedErrMsg string
55-
validateResult func(t *testing.T, response map[string]any)
64+
name string
65+
mockedClient *http.Client
66+
mockedGQLClient *http.Client
67+
requestArgs map[string]any
68+
expectError bool
69+
expectedErrMsg string
70+
validateResult func(t *testing.T, responseText string)
5671
}{
5772
{
5873
name: "successful assignees fetch",
@@ -65,7 +80,9 @@ func Test_UIGet(t *testing.T) {
6580
"repo": "repo",
6681
},
6782
expectError: false,
68-
validateResult: func(t *testing.T, response map[string]any) {
83+
validateResult: func(t *testing.T, responseText string) {
84+
var response map[string]any
85+
require.NoError(t, json.Unmarshal([]byte(responseText), &response))
6986
assert.Contains(t, response, "assignees")
7087
assert.Contains(t, response, "totalCount")
7188
},
@@ -81,11 +98,128 @@ func Test_UIGet(t *testing.T) {
8198
"repo": "repo",
8299
},
83100
expectError: false,
84-
validateResult: func(t *testing.T, response map[string]any) {
101+
validateResult: func(t *testing.T, responseText string) {
102+
var response map[string]any
103+
require.NoError(t, json.Unmarshal([]byte(responseText), &response))
85104
assert.Contains(t, response, "branches")
86105
assert.Contains(t, response, "totalCount")
87106
},
88107
},
108+
{
109+
name: "successful milestones fetch",
110+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
111+
"GET /repos/owner/repo/milestones": mockResponse(t, http.StatusOK, mockMilestones),
112+
}),
113+
requestArgs: map[string]any{
114+
"method": "milestones",
115+
"owner": "owner",
116+
"repo": "repo",
117+
},
118+
expectError: false,
119+
validateResult: func(t *testing.T, responseText string) {
120+
var response map[string]any
121+
require.NoError(t, json.Unmarshal([]byte(responseText), &response))
122+
milestones, ok := response["milestones"].([]any)
123+
require.True(t, ok, "milestones should be a list")
124+
require.Len(t, milestones, 2)
125+
first := milestones[0].(map[string]any)
126+
assert.Equal(t, "2026-01-31", first["due_on"], "milestone with a due date should be formatted")
127+
second := milestones[1].(map[string]any)
128+
assert.Equal(t, "", second["due_on"], "milestone without a due date should be empty, not zero time")
129+
},
130+
},
131+
{
132+
name: "successful issue_types fetch",
133+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
134+
"GET /orgs/owner/issue-types": mockResponse(t, http.StatusOK, mockIssueTypes),
135+
}),
136+
requestArgs: map[string]any{
137+
"method": "issue_types",
138+
"owner": "owner",
139+
},
140+
expectError: false,
141+
validateResult: func(t *testing.T, responseText string) {
142+
var issueTypes []map[string]any
143+
require.NoError(t, json.Unmarshal([]byte(responseText), &issueTypes))
144+
require.Len(t, issueTypes, 2)
145+
assert.Equal(t, "Bug", issueTypes[0]["name"])
146+
},
147+
},
148+
{
149+
name: "issue_types API error returns response context",
150+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
151+
"GET /orgs/owner/issue-types": mockResponse(t, http.StatusForbidden, map[string]string{"message": "Forbidden"}),
152+
}),
153+
requestArgs: map[string]any{
154+
"method": "issue_types",
155+
"owner": "owner",
156+
},
157+
expectError: true,
158+
expectedErrMsg: "failed to list issue types",
159+
},
160+
{
161+
name: "successful labels fetch",
162+
mockedGQLClient: githubv4mock.NewMockedHTTPClient(
163+
githubv4mock.NewQueryMatcher(
164+
struct {
165+
Repository struct {
166+
Labels struct {
167+
Nodes []struct {
168+
ID githubv4.ID
169+
Name githubv4.String
170+
Color githubv4.String
171+
Description githubv4.String
172+
}
173+
TotalCount githubv4.Int
174+
PageInfo struct {
175+
HasNextPage githubv4.Boolean
176+
EndCursor githubv4.String
177+
}
178+
} `graphql:"labels(first: 100, after: $cursor)"`
179+
} `graphql:"repository(owner: $owner, name: $repo)"`
180+
}{},
181+
map[string]any{
182+
"owner": githubv4.String("owner"),
183+
"repo": githubv4.String("repo"),
184+
"cursor": (*githubv4.String)(nil),
185+
},
186+
githubv4mock.DataResponse(map[string]any{
187+
"repository": map[string]any{
188+
"labels": map[string]any{
189+
"nodes": []any{
190+
map[string]any{
191+
"id": githubv4.ID("label-1"),
192+
"name": githubv4.String("bug"),
193+
"color": githubv4.String("d73a4a"),
194+
"description": githubv4.String("Something isn't working"),
195+
},
196+
},
197+
"totalCount": githubv4.Int(1),
198+
"pageInfo": map[string]any{
199+
"hasNextPage": githubv4.Boolean(false),
200+
"endCursor": githubv4.String(""),
201+
},
202+
},
203+
},
204+
}),
205+
),
206+
),
207+
requestArgs: map[string]any{
208+
"method": "labels",
209+
"owner": "owner",
210+
"repo": "repo",
211+
},
212+
expectError: false,
213+
validateResult: func(t *testing.T, responseText string) {
214+
var response map[string]any
215+
require.NoError(t, json.Unmarshal([]byte(responseText), &response))
216+
labels, ok := response["labels"].([]any)
217+
require.True(t, ok, "labels should be a list")
218+
require.Len(t, labels, 1)
219+
assert.Equal(t, "bug", labels[0].(map[string]any)["name"])
220+
assert.Equal(t, float64(1), response["totalCount"])
221+
},
222+
},
89223
{
90224
name: "missing method parameter",
91225
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
@@ -131,11 +265,15 @@ func Test_UIGet(t *testing.T) {
131265

132266
for _, tc := range tests {
133267
t.Run(tc.name, func(t *testing.T) {
134-
// Setup client with mock
135-
client, err := github.NewClient(github.WithHTTPClient(tc.mockedClient))
136-
require.NoError(t, err)
137-
deps := BaseDeps{
138-
Client: client,
268+
// Setup deps with REST and/or GraphQL mocks
269+
deps := BaseDeps{}
270+
if tc.mockedClient != nil {
271+
client, err := github.NewClient(github.WithHTTPClient(tc.mockedClient))
272+
require.NoError(t, err)
273+
deps.Client = client
274+
}
275+
if tc.mockedGQLClient != nil {
276+
deps.GQLClient = githubv4.NewClient(tc.mockedGQLClient)
139277
}
140278
handler := serverTool.Handler(deps)
141279

@@ -163,12 +301,8 @@ func Test_UIGet(t *testing.T) {
163301
require.False(t, result.IsError)
164302
textContent := getTextResult(t, result)
165303

166-
var response map[string]any
167-
err = json.Unmarshal([]byte(textContent.Text), &response)
168-
require.NoError(t, err)
169-
170304
if tc.validateResult != nil {
171-
tc.validateResult(t, response)
305+
tc.validateResult(t, textContent.Text)
172306
}
173307
})
174308
}

0 commit comments

Comments
 (0)