Skip to content

Commit adce445

Browse files
ppofficeppoffice
committed
Address user list review feedback
Fully paginate UserList item reads, including membership discovery before updateUserListsForItem replaces the full list set. Preserve explicit empty descriptions on updates, correct add/remove idempotency and destructiveness annotations, and make e2e cleanup track the active list name after rename. Update focused tests and tool snapshots accordingly.
1 parent b6c27e0 commit adce445

5 files changed

Lines changed: 246 additions & 81 deletions

File tree

‎e2e/e2e_test.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2012,14 +2012,18 @@ func TestUserLists(t *testing.T) {
20122012
listName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli())
20132013
renamedList := listName + "-renamed"
20142014

2015+
// currentListName tracks the list's current name so cleanup deletes the
2016+
// right one even if the rename below fails after creation. It is only
2017+
// advanced once the rename succeeds.
2018+
currentListName := listName
20152019
t.Cleanup(func() {
2016-
t.Logf("Cleaning up list %q...", renamedList)
2020+
t.Logf("Cleaning up list %q...", currentListName)
20172021
resp, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{
20182022
Name: "delete_user_list",
2019-
Arguments: map[string]any{"name": renamedList},
2023+
Arguments: map[string]any{"name": currentListName},
20202024
})
20212025
if err == nil && resp.IsError {
2022-
t.Logf("Cleanup: failed to delete list %q: %+v", renamedList, resp)
2026+
t.Logf("Cleanup: failed to delete list %q: %+v", currentListName, resp)
20232027
}
20242028
})
20252029

@@ -2047,6 +2051,7 @@ func TestUserLists(t *testing.T) {
20472051
})
20482052
require.NoError(t, err, "expected to call 'update_user_list' tool successfully")
20492053
require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp))
2054+
currentListName = renamedList
20502055

20512056
// Use the user's own account to find a repository to add. We create one so
20522057
// the test is self-contained and does not depend on any pre-existing repo.

‎pkg/github/__toolsnaps__/add_repository_to_list.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"annotations": {
33
"destructiveHint": false,
4-
"idempotentHint": false,
4+
"idempotentHint": true,
55
"readOnlyHint": false,
66
"title": "Add repository to star list"
77
},

‎pkg/github/__toolsnaps__/remove_repository_from_list.snap‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"annotations": {
3-
"destructiveHint": false,
4-
"idempotentHint": false,
3+
"destructiveHint": true,
4+
"idempotentHint": true,
55
"readOnlyHint": false,
66
"title": "Remove repository from star list"
77
},

‎pkg/github/user_lists.go‎

Lines changed: 177 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -92,34 +92,91 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo
9292
return lists, int(query.Viewer.Lists.TotalCount), nil
9393
}
9494

95-
// listUserListItems returns the repositories held by a single list.
95+
// listUserListItems returns the repositories held by a single list, following
96+
// the items connection's cursor until every page has been consumed.
9697
func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID) ([]userListItem, error) {
97-
var query struct {
98-
Node struct {
99-
UserList struct {
100-
Items struct {
101-
Nodes []struct {
102-
Repository struct {
103-
NameWithOwner githubv4.String
104-
} `graphql:"... on Repository"`
105-
}
106-
} `graphql:"items(first: 100)"`
107-
} `graphql:"... on UserList"`
108-
} `graphql:"node(id: $id)"`
109-
}
110-
vars := map[string]any{
111-
"id": listID,
112-
}
113-
if err := client.Query(ctx, &query, vars); err != nil {
114-
return nil, err
115-
}
116-
items := make([]userListItem, 0, len(query.Node.UserList.Items.Nodes))
117-
for _, node := range query.Node.UserList.Items.Nodes {
118-
items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)})
98+
items := make([]userListItem, 0)
99+
var after *githubv4.String
100+
for {
101+
var query struct {
102+
Node struct {
103+
UserList struct {
104+
Items struct {
105+
Nodes []struct {
106+
Repository struct {
107+
NameWithOwner githubv4.String
108+
} `graphql:"... on Repository"`
109+
}
110+
PageInfo struct {
111+
HasNextPage bool
112+
EndCursor string
113+
}
114+
} `graphql:"items(first: 100, after: $after)"`
115+
} `graphql:"... on UserList"`
116+
} `graphql:"node(id: $id)"`
117+
}
118+
vars := map[string]any{
119+
"id": listID,
120+
"after": after,
121+
}
122+
if err := client.Query(ctx, &query, vars); err != nil {
123+
return nil, err
124+
}
125+
for _, node := range query.Node.UserList.Items.Nodes {
126+
items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)})
127+
}
128+
if !query.Node.UserList.Items.PageInfo.HasNextPage {
129+
break
130+
}
131+
cursor := githubv4.String(query.Node.UserList.Items.PageInfo.EndCursor)
132+
after = &cursor
119133
}
120134
return items, nil
121135
}
122136

137+
// repoInList reports whether the repository identified by repoID belongs to the
138+
// list identified by listID, paging through the list's items until a match is
139+
// found or the connection is exhausted.
140+
func repoInList(ctx context.Context, client *githubv4.Client, listID, repoID githubv4.ID) (bool, error) {
141+
var after *githubv4.String
142+
for {
143+
var query struct {
144+
Node struct {
145+
UserList struct {
146+
Items struct {
147+
Nodes []struct {
148+
Repository struct {
149+
ID githubv4.ID
150+
} `graphql:"... on Repository"`
151+
}
152+
PageInfo struct {
153+
HasNextPage bool
154+
EndCursor string
155+
}
156+
} `graphql:"items(first: 100, after: $after)"`
157+
} `graphql:"... on UserList"`
158+
} `graphql:"node(id: $id)"`
159+
}
160+
vars := map[string]any{
161+
"id": listID,
162+
"after": after,
163+
}
164+
if err := client.Query(ctx, &query, vars); err != nil {
165+
return false, err
166+
}
167+
for _, node := range query.Node.UserList.Items.Nodes {
168+
if node.Repository.ID == repoID {
169+
return true, nil
170+
}
171+
}
172+
if !query.Node.UserList.Items.PageInfo.HasNextPage {
173+
return false, nil
174+
}
175+
cursor := githubv4.String(query.Node.UserList.Items.PageInfo.EndCursor)
176+
after = &cursor
177+
}
178+
}
179+
123180
// createUserList creates a new star list for the authenticated user.
124181
func createUserList(ctx context.Context, client *githubv4.Client, name, description string, isPrivate *bool) (string, error) {
125182
input := githubv4.CreateUserListInput{
@@ -150,8 +207,10 @@ func createUserList(ctx context.Context, client *githubv4.Client, name, descript
150207

151208
// updateUserList updates the name, description, and/or privacy of an existing
152209
// star list. name identifies the list; newName, description, and isPrivate are
153-
// optional changes.
154-
func updateUserList(ctx context.Context, client *githubv4.Client, name, newName, description string, isPrivate *bool) (string, error) {
210+
// optional changes. description is nil when the field was omitted (leave
211+
// unchanged) and non-nil when supplied (including an explicit empty string,
212+
// which clears the description).
213+
func updateUserList(ctx context.Context, client *githubv4.Client, name, newName string, description *string, isPrivate *bool) (string, error) {
155214
listID, err := getUserListID(ctx, client, name)
156215
if err != nil {
157216
return "", err
@@ -164,8 +223,8 @@ func updateUserList(ctx context.Context, client *githubv4.Client, name, newName,
164223
n := githubv4.String(newName)
165224
input.Name = &n
166225
}
167-
if description != "" {
168-
d := githubv4.String(description)
226+
if description != nil {
227+
d := githubv4.String(*description)
169228
input.Description = &d
170229
}
171230
if isPrivate != nil {
@@ -215,7 +274,9 @@ func deleteUserList(ctx context.Context, client *githubv4.Client, name string) e
215274
// GitHub's schema has no reverse lookup from a repository to its lists (there
216275
// is no `lists` field on Repository). Membership is instead derived by walking
217276
// the viewer's lists and checking each list's items for the repository's node
218-
// ID.
277+
// ID. Both the lists and each list's items are fully paginated so a repository
278+
// beyond the first 100 items of a list is still counted; omitting it here would
279+
// silently drop the repository from that list on the subsequent mutation.
219280
func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, repo, listName string, add bool) error {
220281
listID, err := getUserListID(ctx, client, listName)
221282
if err != nil {
@@ -227,54 +288,97 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner,
227288
return fmt.Errorf("failed to find repository: %w", err)
228289
}
229290

230-
var query struct {
231-
Viewer struct {
232-
Lists struct {
233-
Nodes []struct {
234-
ID githubv4.ID
235-
Items struct {
236-
Nodes []struct {
237-
Repository struct {
238-
ID githubv4.ID
239-
} `graphql:"... on Repository"`
240-
}
241-
} `graphql:"items(first: 100)"`
291+
// Walk every list and, for each, every page of items to determine which
292+
// lists currently contain the repository.
293+
var listIDs []githubv4.ID
294+
var listsAfter *githubv4.String
295+
for {
296+
var query struct {
297+
Viewer struct {
298+
Lists struct {
299+
Nodes []struct {
300+
ID githubv4.ID
301+
Items struct {
302+
Nodes []struct {
303+
Repository struct {
304+
ID githubv4.ID
305+
} `graphql:"... on Repository"`
306+
}
307+
PageInfo struct {
308+
HasNextPage bool
309+
EndCursor string
310+
}
311+
} `graphql:"items(first: 100)"`
312+
}
313+
PageInfo struct {
314+
HasNextPage bool
315+
EndCursor string
316+
}
317+
} `graphql:"lists(first: 100, after: $listsAfter)"`
318+
}
319+
}
320+
vars := map[string]any{
321+
"listsAfter": listsAfter,
322+
}
323+
if err := client.Query(ctx, &query, vars); err != nil {
324+
return err
325+
}
326+
for _, list := range query.Viewer.Lists.Nodes {
327+
contains := false
328+
for _, item := range list.Items.Nodes {
329+
if item.Repository.ID == repoID {
330+
contains = true
331+
break
242332
}
243-
} `graphql:"lists(first: 100)"`
333+
}
334+
// If the first page didn't contain the repository but the list has
335+
// more than 100 items, keep paging until we know for certain.
336+
if !contains && list.Items.PageInfo.HasNextPage {
337+
var err error
338+
contains, err = repoInList(ctx, client, list.ID, repoID)
339+
if err != nil {
340+
return err
341+
}
342+
}
343+
if contains {
344+
listIDs = append(listIDs, list.ID)
345+
}
244346
}
245-
}
246-
if err := client.Query(ctx, &query, nil); err != nil {
247-
return err
347+
348+
if !query.Viewer.Lists.PageInfo.HasNextPage {
349+
break
350+
}
351+
cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor)
352+
listsAfter = &cursor
248353
}
249354

250-
listIDs := make([]githubv4.ID, 0, len(query.Viewer.Lists.Nodes)+1)
251355
present := false
252-
for _, list := range query.Viewer.Lists.Nodes {
253-
contains := false
254-
for _, item := range list.Items.Nodes {
255-
if item.Repository.ID == repoID {
256-
contains = true
257-
break
258-
}
356+
for _, id := range listIDs {
357+
if id == listID {
358+
present = true
359+
break
259360
}
260-
if !contains {
261-
continue
361+
}
362+
363+
result := make([]githubv4.ID, 0, len(listIDs)+1)
364+
if add {
365+
for _, id := range listIDs {
366+
result = append(result, id)
262367
}
263-
if list.ID == listID {
264-
present = true
265-
if !add {
266-
continue
368+
if !present {
369+
result = append(result, listID)
370+
}
371+
} else {
372+
for _, id := range listIDs {
373+
if id != listID {
374+
result = append(result, id)
267375
}
268376
}
269-
listIDs = append(listIDs, list.ID)
270-
}
271-
if add && !present {
272-
listIDs = append(listIDs, listID)
273377
}
274378

275379
input := githubv4.UpdateUserListsForItemInput{
276380
ItemID: repoID,
277-
ListIDs: listIDs,
381+
ListIDs: result,
278382
}
279383
var mutation struct {
280384
UpdateUserListsForItem struct {
@@ -447,7 +551,7 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool {
447551
if err != nil {
448552
return utils.NewToolResultError(err.Error()), nil, nil
449553
}
450-
description, err := OptionalParam[string](args, "description")
554+
description, descPresent, err := OptionalParamOK[string](args, "description")
451555
if err != nil {
452556
return utils.NewToolResultError(err.Error()), nil, nil
453557
}
@@ -456,7 +560,7 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool {
456560
return utils.NewToolResultError(err.Error()), nil, nil
457561
}
458562

459-
if newName == "" && description == "" && !present {
563+
if newName == "" && !descPresent && !present {
460564
return utils.NewToolResultError("at least one of new_name, description, or is_private must be provided for update"), nil, nil
461565
}
462566

@@ -469,7 +573,11 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool {
469573
if present {
470574
isPrivatePtr = &isPrivate
471575
}
472-
updatedName, err := updateUserList(ctx, client, name, newName, description, isPrivatePtr)
576+
var descriptionPtr *string
577+
if descPresent {
578+
descriptionPtr = &description
579+
}
580+
updatedName, err := updateUserList(ctx, client, name, newName, descriptionPtr, isPrivatePtr)
473581
if err != nil {
474582
return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to update user list", err), nil, nil
475583
}
@@ -532,6 +640,7 @@ func AddRepositoryToList(t translations.TranslationHelperFunc) inventory.ServerT
532640
Title: t("TOOL_ADD_REPOSITORY_TO_LIST_USER_TITLE", "Add repository to star list"),
533641
ReadOnlyHint: false,
534642
DestructiveHint: jsonschema.Ptr(false),
643+
IdempotentHint: true,
535644
},
536645
InputSchema: &jsonschema.Schema{
537646
Type: "object",
@@ -590,7 +699,8 @@ func RemoveRepositoryFromList(t translations.TranslationHelperFunc) inventory.Se
590699
Annotations: &mcp.ToolAnnotations{
591700
Title: t("TOOL_REMOVE_REPOSITORY_FROM_LIST_USER_TITLE", "Remove repository from star list"),
592701
ReadOnlyHint: false,
593-
DestructiveHint: jsonschema.Ptr(false),
702+
DestructiveHint: jsonschema.Ptr(true),
703+
IdempotentHint: true,
594704
},
595705
InputSchema: &jsonschema.Schema{
596706
Type: "object",

0 commit comments

Comments
 (0)