From 523e804491d110bc36dde4dec3e49859816a1888 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Sun, 30 Aug 2026 08:31:56 -0700 Subject: [PATCH 01/12] Add star list (UserList) management tools Adds 6 tools to the stargazers toolset for managing GitHub star lists (the UserList feature at github.com/stars): list_user_lists, create_user_list, update_user_list, delete_user_list, add_repository_to_list, and remove_repository_from_list. List membership is independent of star state, so the add/remove tools perform a read-modify-write against updateUserListsForItem (which REPLACES membership) without any star/unstar side-effects. Adds the 'user' OAuth scope as opt-in (not in the default set). --- README.md | 33 + .../__toolsnaps__/add_repository_to_list.snap | 32 + .../__toolsnaps__/create_user_list.snap | 30 + .../__toolsnaps__/delete_user_list.snap | 22 + pkg/github/__toolsnaps__/list_user_lists.snap | 18 + .../remove_repository_from_list.snap | 32 + .../__toolsnaps__/update_user_list.snap | 34 + pkg/github/star_lists.go | 622 ++++++++++++++ pkg/github/star_lists_test.go | 805 ++++++++++++++++++ pkg/github/tools.go | 6 + pkg/http/oauth/oauth_test.go | 1 + pkg/scopes/scopes.go | 1 + 12 files changed, 1636 insertions(+) create mode 100644 pkg/github/__toolsnaps__/add_repository_to_list.snap create mode 100644 pkg/github/__toolsnaps__/create_user_list.snap create mode 100644 pkg/github/__toolsnaps__/delete_user_list.snap create mode 100644 pkg/github/__toolsnaps__/list_user_lists.snap create mode 100644 pkg/github/__toolsnaps__/remove_repository_from_list.snap create mode 100644 pkg/github/__toolsnaps__/update_user_list.snap create mode 100644 pkg/github/star_lists.go create mode 100644 pkg/github/star_lists_test.go diff --git a/README.md b/README.md index 87965cb09d..be7e59202e 100644 --- a/README.md +++ b/README.md @@ -1481,6 +1481,22 @@ The following sets of tools are available: star Stargazers +- **add_repository_to_list** - Add repository to star list + - **OAuth Challenge Scopes**: `user` + - `list_name`: The name of the star list to add the repository to. (string, required) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + +- **create_user_list** - Create star list + - **OAuth Challenge Scopes**: `user` + - `description`: A description of the list. (string, optional) + - `is_private`: Whether the list is private. (boolean, optional) + - `name`: The name of the new list. (string, required) + +- **delete_user_list** - Delete star list + - **OAuth Challenge Scopes**: `user` + - `name`: The name of the list to delete. (string, required) + - **list_starred_repositories** - List starred repositories - **OAuth Challenge Scopes**: `repo` - `direction`: The direction to sort the results by. (string, optional) @@ -1489,6 +1505,16 @@ The following sets of tools are available: - `sort`: How to sort the results. Can be either 'created' (when the repository was starred) or 'updated' (when the repository was last pushed to). (string, optional) - `username`: Username to list starred repositories for. Defaults to the authenticated user. (string, optional) +- **list_user_lists** - List star lists + - **OAuth Challenge Scopes**: `read:user` + - `include_items`: Whether to include the repositories in each list. (boolean, optional) + +- **remove_repository_from_list** - Remove repository from star list + - **OAuth Challenge Scopes**: `user` + - `list_name`: The name of the star list to remove the repository from. (string, required) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - **star_repository** - Star repository - **OAuth Challenge Scopes**: `repo` - `owner`: Repository owner (string, required) @@ -1499,6 +1525,13 @@ The following sets of tools are available: - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) +- **update_user_list** - Update star list + - **OAuth Challenge Scopes**: `user` + - `description`: The new description for the list. (string, optional) + - `is_private`: Whether the list is private. (boolean, optional) + - `name`: The current name of the list to update. (string, required) + - `new_name`: The new name for the list. (string, optional) +
diff --git a/pkg/github/__toolsnaps__/add_repository_to_list.snap b/pkg/github/__toolsnaps__/add_repository_to_list.snap new file mode 100644 index 0000000000..31f69de696 --- /dev/null +++ b/pkg/github/__toolsnaps__/add_repository_to_list.snap @@ -0,0 +1,32 @@ +{ + "annotations": { + "destructiveHint": false, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Add repository to star list" + }, + "description": "Add a repository to a star list (UserList). List membership is independent of star state.", + "inputSchema": { + "properties": { + "list_name": { + "description": "The name of the star list to add the repository to.", + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "list_name" + ], + "type": "object" + }, + "name": "add_repository_to_list" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/create_user_list.snap b/pkg/github/__toolsnaps__/create_user_list.snap new file mode 100644 index 0000000000..e7bfd91fb8 --- /dev/null +++ b/pkg/github/__toolsnaps__/create_user_list.snap @@ -0,0 +1,30 @@ +{ + "annotations": { + "destructiveHint": false, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Create star list" + }, + "description": "Create a new star list (UserList) for the authenticated user.", + "inputSchema": { + "properties": { + "description": { + "description": "A description of the list.", + "type": "string" + }, + "is_private": { + "description": "Whether the list is private.", + "type": "boolean" + }, + "name": { + "description": "The name of the new list.", + "type": "string" + } + }, + "required": [ + "name" + ], + "type": "object" + }, + "name": "create_user_list" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/delete_user_list.snap b/pkg/github/__toolsnaps__/delete_user_list.snap new file mode 100644 index 0000000000..529e8b3b23 --- /dev/null +++ b/pkg/github/__toolsnaps__/delete_user_list.snap @@ -0,0 +1,22 @@ +{ + "annotations": { + "destructiveHint": true, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Delete star list" + }, + "description": "Delete a star list (UserList) owned by the authenticated user.", + "inputSchema": { + "properties": { + "name": { + "description": "The name of the list to delete.", + "type": "string" + } + }, + "required": [ + "name" + ], + "type": "object" + }, + "name": "delete_user_list" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_user_lists.snap b/pkg/github/__toolsnaps__/list_user_lists.snap new file mode 100644 index 0000000000..8ca3ca213c --- /dev/null +++ b/pkg/github/__toolsnaps__/list_user_lists.snap @@ -0,0 +1,18 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "List star lists" + }, + "description": "List the authenticated user's star lists (UserLists), optionally including the repositories in each list.", + "inputSchema": { + "properties": { + "include_items": { + "description": "Whether to include the repositories in each list.", + "type": "boolean" + } + }, + "type": "object" + }, + "name": "list_user_lists" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/remove_repository_from_list.snap b/pkg/github/__toolsnaps__/remove_repository_from_list.snap new file mode 100644 index 0000000000..e421d5d965 --- /dev/null +++ b/pkg/github/__toolsnaps__/remove_repository_from_list.snap @@ -0,0 +1,32 @@ +{ + "annotations": { + "destructiveHint": false, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Remove repository from star list" + }, + "description": "Remove a repository from a star list (UserList). List membership is independent of star state.", + "inputSchema": { + "properties": { + "list_name": { + "description": "The name of the star list to remove the repository from.", + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "list_name" + ], + "type": "object" + }, + "name": "remove_repository_from_list" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_user_list.snap b/pkg/github/__toolsnaps__/update_user_list.snap new file mode 100644 index 0000000000..17978b9a51 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_user_list.snap @@ -0,0 +1,34 @@ +{ + "annotations": { + "destructiveHint": false, + "idempotentHint": false, + "readOnlyHint": false, + "title": "Update star list" + }, + "description": "Update an existing star list (UserList): rename it, change its description, or change its privacy.", + "inputSchema": { + "properties": { + "description": { + "description": "The new description for the list.", + "type": "string" + }, + "is_private": { + "description": "Whether the list is private.", + "type": "boolean" + }, + "name": { + "description": "The current name of the list to update.", + "type": "string" + }, + "new_name": { + "description": "The new name for the list.", + "type": "string" + } + }, + "required": [ + "name" + ], + "type": "object" + }, + "name": "update_user_list" +} \ No newline at end of file diff --git a/pkg/github/star_lists.go b/pkg/github/star_lists.go new file mode 100644 index 0000000000..f543d3b3b7 --- /dev/null +++ b/pkg/github/star_lists.go @@ -0,0 +1,622 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/shurcooL/githubv4" +) + +// userList represents a GitHub star list (UserList) surfaced through the tools. +type userList struct { + ID githubv4.ID `json:"id"` + Name string `json:"name"` + Description string `json:"description,omitempty"` + IsPrivate bool `json:"is_private"` + Items []userListItem `json:"items,omitempty"` +} + +type userListItem struct { + Repository string `json:"repository"` +} + +// getUserListID resolves the authenticated user's list with the given name to +// its node ID. It returns an error when no list matches the name. +func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { + var query struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + } + if err := client.Query(ctx, &query, nil); err != nil { + return "", err + } + for _, node := range query.Viewer.Lists.Nodes { + if string(node.Name) == name { + return node.ID, nil + } + } + return "", fmt.Errorf("list '%s' not found", name) +} + +// listUserLists returns the authenticated user's star lists, optionally +// including the repositories each list contains. +func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bool) ([]userList, int, error) { + var query struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100)"` + } + } + if err := client.Query(ctx, &query, nil); err != nil { + return nil, 0, err + } + + lists := make([]userList, 0, len(query.Viewer.Lists.Nodes)) + for _, node := range query.Viewer.Lists.Nodes { + list := userList{ + ID: node.ID, + Name: string(node.Name), + Description: string(node.Description), + IsPrivate: bool(node.IsPrivate), + } + if includeItems { + items, err := listUserListItems(ctx, client, node.ID) + if err != nil { + return nil, 0, err + } + list.Items = items + } + lists = append(lists, list) + } + return lists, int(query.Viewer.Lists.TotalCount), nil +} + +// listUserListItems returns the repositories held by a single list. +func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID) ([]userListItem, error) { + var query struct { + Node struct { + UserList struct { + Items struct { + Nodes []struct { + Repository struct { + NameWithOwner githubv4.String + } + } + } `graphql:"items(first: 100)"` + } `graphql:"... on UserList"` + } `graphql:"node(id: $id)"` + } + vars := map[string]any{ + "id": listID, + } + if err := client.Query(ctx, &query, vars); err != nil { + return nil, err + } + items := make([]userListItem, 0, len(query.Node.UserList.Items.Nodes)) + for _, node := range query.Node.UserList.Items.Nodes { + items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)}) + } + return items, nil +} + +// createUserList creates a new star list for the authenticated user. +func createUserList(ctx context.Context, client *githubv4.Client, name, description string, isPrivate *bool) (string, error) { + input := githubv4.CreateUserListInput{ + Name: githubv4.String(name), + } + if description != "" { + d := githubv4.String(description) + input.Description = &d + } + if isPrivate != nil { + p := githubv4.Boolean(*isPrivate) + input.IsPrivate = &p + } + + var mutation struct { + CreateUserList struct { + List struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"createUserList(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return "", err + } + return string(mutation.CreateUserList.List.Name), nil +} + +// updateUserList updates the name, description, and/or privacy of an existing +// star list. name identifies the list; newName, description, and isPrivate are +// optional changes. +func updateUserList(ctx context.Context, client *githubv4.Client, name, newName, description string, isPrivate *bool) (string, error) { + listID, err := getUserListID(ctx, client, name) + if err != nil { + return "", err + } + + input := githubv4.UpdateUserListInput{ + ListID: listID, + } + if newName != "" { + n := githubv4.String(newName) + input.Name = &n + } + if description != "" { + d := githubv4.String(description) + input.Description = &d + } + if isPrivate != nil { + p := githubv4.Boolean(*isPrivate) + input.IsPrivate = &p + } + + var mutation struct { + UpdateUserList struct { + List struct { + Name githubv4.String + } + } `graphql:"updateUserList(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return "", err + } + return string(mutation.UpdateUserList.List.Name), nil +} + +// deleteUserList deletes a star list owned by the authenticated user. +func deleteUserList(ctx context.Context, client *githubv4.Client, name string) error { + listID, err := getUserListID(ctx, client, name) + if err != nil { + return err + } + + input := githubv4.DeleteUserListInput{ + ListID: listID, + } + var mutation struct { + DeleteUserList struct { + ClientMutationID githubv4.String + } `graphql:"deleteUserList(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return err + } + return nil +} + +// setRepoListMemberships adds (add=true) or removes (add=false) a repository +// from the named list. updateUserListsForItem REPLACES the repository's full +// list membership, so the current set is read first, merged/subtracted, and +// resubmitted in full. +func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, repo, listName string, add bool) error { + listID, err := getUserListID(ctx, client, listName) + if err != nil { + return err + } + + repoID, err := getRepositoryID(ctx, client, owner, repo) + if err != nil { + return fmt.Errorf("failed to find repository: %w", err) + } + + var repoQuery struct { + Repository struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + } + } `graphql:"lists(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + vars := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + } + if err := client.Query(ctx, &repoQuery, vars); err != nil { + return err + } + + listIDs := make([]githubv4.ID, 0, len(repoQuery.Repository.Lists.Nodes)+1) + present := false + for _, node := range repoQuery.Repository.Lists.Nodes { + if node.ID == listID { + present = true + if !add { + continue + } + } + listIDs = append(listIDs, node.ID) + } + if add && !present { + listIDs = append(listIDs, listID) + } + + input := githubv4.UpdateUserListsForItemInput{ + ItemID: repoID, + ListIDs: listIDs, + } + var mutation struct { + UpdateUserListsForItem struct { + ClientMutationID githubv4.String + } `graphql:"updateUserListsForItem(input: $input)"` + } + if err := client.Mutate(ctx, &mutation, input, nil); err != nil { + return err + } + return nil +} + +// ListUserLists creates a tool to list the authenticated user's star lists. +func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "list_user_lists", + Description: t("TOOL_LIST_USER_LISTS_DESCRIPTION", "List the authenticated user's star lists (UserLists), optionally including the repositories in each list."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_USER_LISTS_USER_TITLE", "List star lists"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "include_items": { + Type: "boolean", + Description: "Whether to include the repositories in each list.", + }, + }, + }, + }, + scopes.PublicRead(scopes.ReadUser), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + includeItems, err := OptionalParam[bool](args, "include_items") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + lists, totalCount, err := listUserLists(ctx, client, includeItems) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to list user lists", err), nil, nil + } + + response := map[string]any{ + "lists": lists, + "totalCount": totalCount, + } + out, err := json.Marshal(response) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal user lists: %w", err) + } + return utils.NewToolResultText(string(out)), nil, nil + }, + ) +} + +// CreateUserList creates a tool to create a new star list. +func CreateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "create_user_list", + Description: t("TOOL_CREATE_USER_LIST_DESCRIPTION", "Create a new star list (UserList) for the authenticated user."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_CREATE_USER_LIST_USER_TITLE", "Create star list"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The name of the new list.", + }, + "description": { + Type: "string", + Description: "A description of the list.", + }, + "is_private": { + Type: "boolean", + Description: "Whether the list is private.", + }, + }, + Required: []string{"name"}, + }, + }, + scopes.RequireAll(scopes.User), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + name, err := RequiredParam[string](args, "name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + description, err := OptionalParam[string](args, "description") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + isPrivate, present, err := OptionalParamOK[bool](args, "is_private") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + var isPrivatePtr *bool + if present { + isPrivatePtr = &isPrivate + } + createdName, err := createUserList(ctx, client, name, description, isPrivatePtr) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to create user list", err), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("list '%s' created successfully", createdName)), nil, nil + }, + ) +} + +// UpdateUserList creates a tool to update an existing star list. +func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "update_user_list", + Description: t("TOOL_UPDATE_USER_LIST_DESCRIPTION", "Update an existing star list (UserList): rename it, change its description, or change its privacy."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UPDATE_USER_LIST_USER_TITLE", "Update star list"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The current name of the list to update.", + }, + "new_name": { + Type: "string", + Description: "The new name for the list.", + }, + "description": { + Type: "string", + Description: "The new description for the list.", + }, + "is_private": { + Type: "boolean", + Description: "Whether the list is private.", + }, + }, + Required: []string{"name"}, + }, + }, + scopes.RequireAll(scopes.User), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + name, err := RequiredParam[string](args, "name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + newName, err := OptionalParam[string](args, "new_name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + description, err := OptionalParam[string](args, "description") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + isPrivate, present, err := OptionalParamOK[bool](args, "is_private") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + if newName == "" && description == "" && !present { + return utils.NewToolResultError("at least one of new_name, description, or is_private must be provided for update"), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + var isPrivatePtr *bool + if present { + isPrivatePtr = &isPrivate + } + updatedName, err := updateUserList(ctx, client, name, newName, description, isPrivatePtr) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to update user list", err), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("list '%s' updated successfully", updatedName)), nil, nil + }, + ) +} + +// DeleteUserList creates a tool to delete a star list. +func DeleteUserList(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "delete_user_list", + Description: t("TOOL_DELETE_USER_LIST_DESCRIPTION", "Delete a star list (UserList) owned by the authenticated user."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_DELETE_USER_LIST_USER_TITLE", "Delete star list"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(true), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The name of the list to delete.", + }, + }, + Required: []string{"name"}, + }, + }, + scopes.RequireAll(scopes.User), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + name, err := RequiredParam[string](args, "name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + if err := deleteUserList(ctx, client, name); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to delete user list", err), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("list '%s' deleted successfully", name)), nil, nil + }, + ) +} + +// AddRepositoryToList creates a tool to add a repository to a star list. +func AddRepositoryToList(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "add_repository_to_list", + Description: t("TOOL_ADD_REPOSITORY_TO_LIST_DESCRIPTION", "Add a repository to a star list (UserList). List membership is independent of star state."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ADD_REPOSITORY_TO_LIST_USER_TITLE", "Add repository to star list"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "list_name": { + Type: "string", + Description: "The name of the star list to add the repository to.", + }, + }, + Required: []string{"owner", "repo", "list_name"}, + }, + }, + scopes.RequireAll(scopes.User), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + listName, err := RequiredParam[string](args, "list_name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + if err := setRepoListMemberships(ctx, client, owner, repo, listName, true); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to add repository to list", err), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("repository %s/%s added to list '%s'", owner, repo, listName)), nil, nil + }, + ) +} + +// RemoveRepositoryFromList creates a tool to remove a repository from a star list. +func RemoveRepositoryFromList(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "remove_repository_from_list", + Description: t("TOOL_REMOVE_REPOSITORY_FROM_LIST_DESCRIPTION", "Remove a repository from a star list (UserList). List membership is independent of star state."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_REMOVE_REPOSITORY_FROM_LIST_USER_TITLE", "Remove repository from star list"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "list_name": { + Type: "string", + Description: "The name of the star list to remove the repository from.", + }, + }, + Required: []string{"owner", "repo", "list_name"}, + }, + }, + scopes.RequireAll(scopes.User), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + listName, err := RequiredParam[string](args, "list_name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + if err := setRepoListMemberships(ctx, client, owner, repo, listName, false); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to remove repository from list", err), nil, nil + } + return utils.NewToolResultText(fmt.Sprintf("repository %s/%s removed from list '%s'", owner, repo, listName)), nil, nil + }, + ) +} diff --git a/pkg/github/star_lists_test.go b/pkg/github/star_lists_test.go new file mode 100644 index 0000000000..7139e11e2e --- /dev/null +++ b/pkg/github/star_lists_test.go @@ -0,0 +1,805 @@ +package github + +import ( + "context" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestListUserLists(t *testing.T) { + t.Parallel() + + serverTool := ListUserLists(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "list_user_lists", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.True(t, tool.Annotations.ReadOnlyHint, "list_user_lists tool should be read-only") + + // scope gating: PublicRead(ReadUser) + assert.Equal(t, []string{"read:user"}, serverTool.ScopeAccess.Scopes) + assert.NotNil(t, serverTool.ScopeAccess.Visible) + assert.NotNil(t, serverTool.ScopeAccess.Challenge) + assert.True(t, serverTool.ScopeAccess.Visible(nil)) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + }{ + { + name: "list user lists without items", + requestArgs: map[string]any{ + "include_items": false, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("list-1"), + "name": githubv4.String("My list"), + "description": githubv4.String("A list"), + "isPrivate": githubv4.Boolean(true), + }, + }, + "totalCount": githubv4.Int(1), + }, + }, + }), + ), + ), + expectToolError: false, + }, + { + name: "list user lists with items", + requestArgs: map[string]any{ + "include_items": true, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("list-1"), + "name": githubv4.String("My list"), + "description": githubv4.String("A list"), + "isPrivate": githubv4.Boolean(false), + }, + }, + "totalCount": githubv4.Int(1), + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Node struct { + UserList struct { + Items struct { + Nodes []struct { + Repository struct { + NameWithOwner githubv4.String + } + } + } `graphql:"items(first: 100)"` + } `graphql:"... on UserList"` + } `graphql:"node(id: $id)"` + }{}, + map[string]any{ + "id": githubv4.ID("list-1"), + }, + githubv4mock.DataResponse(map[string]any{ + "node": map[string]any{ + "items": map[string]any{ + "nodes": []any{ + map[string]any{ + "repository": map[string]any{ + "nameWithOwner": githubv4.String("owner/repo"), + }, + }, + }, + }, + }, + }), + ), + ), + expectToolError: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + if tc.expectToolError { + assert.True(t, result.IsError) + } else { + assert.False(t, result.IsError) + } + }) + } +} + +func TestCreateUserList(t *testing.T) { + t.Parallel() + + serverTool := CreateUserList(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "create_user_list", tool.Name) + assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.False(t, *tool.Annotations.DestructiveHint) + + // scope gating: RequireAll(User) + assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + assert.NotNil(t, serverTool.ScopeAccess.Visible) + assert.NotNil(t, serverTool.ScopeAccess.Challenge) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + }{ + { + name: "create list", + requestArgs: map[string]any{ + "name": "My list", + "description": "A list", + "is_private": true, + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewMutationMatcher( + struct { + CreateUserList struct { + List struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"createUserList(input: $input)"` + }{}, + githubv4.CreateUserListInput{ + Name: githubv4.String("My list"), + Description: func() *githubv4.String { s := githubv4.String("A list"); return &s }(), + IsPrivate: func() *githubv4.Boolean { b := githubv4.Boolean(true); return &b }(), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "createUserList": map[string]any{ + "list": map[string]any{ + "id": githubv4.ID("list-1"), + "name": githubv4.String("My list"), + }, + }, + }), + ), + ), + expectToolError: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + assert.Equal(t, tc.expectToolError, result.IsError) + }) + } +} + +func TestUpdateUserList(t *testing.T) { + t.Parallel() + + serverTool := UpdateUserList(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "update_user_list", tool.Name) + assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.False(t, *tool.Annotations.DestructiveHint) + assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedToolErrMsg string + }{ + { + name: "update list name", + requestArgs: map[string]any{ + "name": "Old name", + "new_name": "New name", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("list-1"), + "name": githubv4.String("Old name"), + }, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + UpdateUserList struct { + List struct { + Name githubv4.String + } + } `graphql:"updateUserList(input: $input)"` + }{}, + githubv4.UpdateUserListInput{ + ListID: githubv4.ID("list-1"), + Name: func() *githubv4.String { s := githubv4.String("New name"); return &s }(), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateUserList": map[string]any{ + "list": map[string]any{ + "name": githubv4.String("New name"), + }, + }, + }), + ), + ), + expectToolError: false, + }, + { + name: "update list not found", + requestArgs: map[string]any{ + "name": "Missing", + "new_name": "New name", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{}, + }, + }, + }), + ), + ), + expectToolError: true, + expectedToolErrMsg: "list 'Missing' not found", + }, + { + name: "update without changes", + requestArgs: map[string]any{ + "name": "My list", + }, + mockedClient: githubv4mock.NewMockedHTTPClient(), + expectToolError: true, + expectedToolErrMsg: "at least one of new_name, description, or is_private must be provided for update", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + if tc.expectToolError { + assert.True(t, result.IsError) + if tc.expectedToolErrMsg != "" { + textContent := getErrorResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedToolErrMsg) + } + } else { + assert.False(t, result.IsError) + } + }) + } +} + +func TestDeleteUserList(t *testing.T) { + t.Parallel() + + serverTool := DeleteUserList(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "delete_user_list", tool.Name) + assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.True(t, *tool.Annotations.DestructiveHint) + assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + + tests := []struct { + name string + requestArgs map[string]any + mockedClient *http.Client + expectToolError bool + expectedToolErrMsg string + }{ + { + name: "delete list", + requestArgs: map[string]any{ + "name": "My list", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": githubv4.ID("list-1"), + "name": githubv4.String("My list"), + }, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + DeleteUserList struct { + ClientMutationID githubv4.String + } `graphql:"deleteUserList(input: $input)"` + }{}, + githubv4.DeleteUserListInput{ + ListID: githubv4.ID("list-1"), + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "deleteUserList": map[string]any{ + "clientMutationId": githubv4.String("test-mutation-id"), + }, + }), + ), + ), + expectToolError: false, + }, + { + name: "delete list not found", + requestArgs: map[string]any{ + "name": "Missing", + }, + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{}, + }, + }, + }), + ), + ), + expectToolError: true, + expectedToolErrMsg: "list 'Missing' not found", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := githubv4.NewClient(tc.mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + if tc.expectToolError { + assert.True(t, result.IsError) + if tc.expectedToolErrMsg != "" { + textContent := getErrorResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedToolErrMsg) + } + } else { + assert.False(t, result.IsError) + } + }) + } +} + +func TestAddRepositoryToList(t *testing.T) { + t.Parallel() + + serverTool := AddRepositoryToList(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "add_repository_to_list", tool.Name) + assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.False(t, *tool.Annotations.DestructiveHint) + assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + + // Repository currently in lists "A" and "B"; adding to "C" must resubmit all + // three because updateUserListsForItem REPLACES membership (does not append). + mockedClient := githubv4mock.NewMockedHTTPClient( + // 1. resolve list "C" -> list-c + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("list-c"), "name": githubv4.String("C")}, + }, + }, + }, + }), + ), + // 2. resolve repository -> repo-id + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + ID githubv4.ID + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "id": githubv4.ID("repo-id"), + }, + }), + ), + // 3. read current list membership -> A, B + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + } + } `graphql:"lists(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("list-a")}, + map[string]any{"id": githubv4.ID("list-b")}, + }, + }, + }, + }), + ), + // 4. resubmit full set A, B, C + githubv4mock.NewMutationMatcher( + struct { + UpdateUserListsForItem struct { + ClientMutationID githubv4.String + } `graphql:"updateUserListsForItem(input: $input)"` + }{}, + githubv4.UpdateUserListsForItemInput{ + ItemID: githubv4.ID("repo-id"), + ListIDs: []githubv4.ID{"list-a", "list-b", "list-c"}, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateUserListsForItem": map[string]any{ + "clientMutationId": githubv4.String("test-mutation-id"), + }, + }), + ), + ) + + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "list_name": "C", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + assert.False(t, result.IsError) +} + +func TestRemoveRepositoryFromList(t *testing.T) { + t.Parallel() + + serverTool := RemoveRepositoryFromList(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "remove_repository_from_list", tool.Name) + assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.False(t, *tool.Annotations.DestructiveHint) + assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + + // Repository currently in lists "A" and "B"; removing from "B" must resubmit + // only the remainder ("A"). + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("list-b"), "name": githubv4.String("B")}, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + ID githubv4.ID + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "id": githubv4.ID("repo-id"), + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + } + } `graphql:"lists(first: 100)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "lists": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("list-a")}, + map[string]any{"id": githubv4.ID("list-b")}, + }, + }, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + UpdateUserListsForItem struct { + ClientMutationID githubv4.String + } `graphql:"updateUserListsForItem(input: $input)"` + }{}, + githubv4.UpdateUserListsForItemInput{ + ItemID: githubv4.ID("repo-id"), + ListIDs: []githubv4.ID{"list-a"}, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "updateUserListsForItem": map[string]any{ + "clientMutationId": githubv4.String("test-mutation-id"), + }, + }), + ), + ) + + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "list_name": "B", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + assert.False(t, result.IsError) +} + +func TestAddRepositoryToListListNotFound(t *testing.T) { + t.Parallel() + + serverTool := AddRepositoryToList(translations.NullTranslationHelper) + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{}, + }, + }, + }), + ), + ) + + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{ + GQLClient: client, + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "list_name": "Missing", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + assert.NotNil(t, result) + assert.True(t, result.IsError) + textContent := getErrorResult(t, result) + assert.Contains(t, textContent.Text, "list 'Missing' not found") +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index ca46deadd2..78b9256897 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -241,6 +241,12 @@ func AllTools(t translations.TranslationHelperFunc, opts ...ToolOption) []invent ListStarredRepositories(t), StarRepository(t), UnstarRepository(t), + ListUserLists(t), + CreateUserList(t), + UpdateUserList(t), + DeleteUserList(t), + AddRepositoryToList(t), + RemoveRepositoryFromList(t), ListRepositoryCollaborators(t), // Git tools diff --git a/pkg/http/oauth/oauth_test.go b/pkg/http/oauth/oauth_test.go index 39c7e953b4..e3cbec884b 100644 --- a/pkg/http/oauth/oauth_test.go +++ b/pkg/http/oauth/oauth_test.go @@ -663,6 +663,7 @@ func TestSupportedScopes(t *testing.T) { "notifications", "workflow", "codespace", + "user", } assert.Equal(t, expectedScopes, SupportedScopes) diff --git a/pkg/scopes/scopes.go b/pkg/scopes/scopes.go index d845cc6dc8..b89d009a44 100644 --- a/pkg/scopes/scopes.go +++ b/pkg/scopes/scopes.go @@ -87,6 +87,7 @@ var oauthScopeDefinitions = []oauthScopeDefinition{ {scope: Notifications, byDefault: true}, {scope: Workflow}, {scope: Codespace}, + {scope: User}, } // SupportedOAuthScopes returns every OAuth scope the server may request. From 4586ef7bb3441a8d6751f80b0a6a02b8b1009f56 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Sun, 30 Aug 2026 13:50:41 -0700 Subject: [PATCH 02/12] Rename star_lists files to user_lists for naming consistency --- pkg/github/{star_lists.go => user_lists.go} | 0 pkg/github/{star_lists_test.go => user_lists_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename pkg/github/{star_lists.go => user_lists.go} (100%) rename pkg/github/{star_lists_test.go => user_lists_test.go} (100%) diff --git a/pkg/github/star_lists.go b/pkg/github/user_lists.go similarity index 100% rename from pkg/github/star_lists.go rename to pkg/github/user_lists.go diff --git a/pkg/github/star_lists_test.go b/pkg/github/user_lists_test.go similarity index 100% rename from pkg/github/star_lists_test.go rename to pkg/github/user_lists_test.go From 3ebf1d44f4d43d4c45127cf3085170023244bb1e Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:05:31 -0700 Subject: [PATCH 03/12] Fix add/remove repository list membership read-modify-write GitHub's GraphQL schema has no reverse lookup from a repository to its lists (Repository has no 'lists' field), so querying repository(owner,name){ lists } fails. Derive membership by walking the viewer's lists and checking each list's items for the repository's node ID, then merge/subtract and resubmit the full set. --- pkg/github/user_lists.go | 46 ++++++++++++------ pkg/github/user_lists_test.go | 90 +++++++++++++++++++++++++---------- 2 files changed, 97 insertions(+), 39 deletions(-) diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index f543d3b3b7..8eb3dfd7c3 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -101,7 +101,7 @@ func listUserListItems(ctx context.Context, client *githubv4.Client, listID gith Nodes []struct { Repository struct { NameWithOwner githubv4.String - } + } `graphql:"... on Repository"` } } `graphql:"items(first: 100)"` } `graphql:"... on UserList"` @@ -211,6 +211,11 @@ func deleteUserList(ctx context.Context, client *githubv4.Client, name string) e // from the named list. updateUserListsForItem REPLACES the repository's full // list membership, so the current set is read first, merged/subtracted, and // resubmitted in full. +// +// GitHub's schema has no reverse lookup from a repository to its lists (there +// is no `lists` field on Repository). Membership is instead derived by walking +// the viewer's lists and checking each list's items for the repository's node +// ID. func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, repo, listName string, add bool) error { listID, err := getUserListID(ctx, client, listName) if err != nil { @@ -222,33 +227,46 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, return fmt.Errorf("failed to find repository: %w", err) } - var repoQuery struct { - Repository struct { + var query struct { + Viewer struct { Lists struct { Nodes []struct { - ID githubv4.ID + ID githubv4.ID + Items struct { + Nodes []struct { + Repository struct { + ID githubv4.ID + } `graphql:"... on Repository"` + } + } `graphql:"items(first: 100)"` } } `graphql:"lists(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - vars := map[string]any{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), + } } - if err := client.Query(ctx, &repoQuery, vars); err != nil { + if err := client.Query(ctx, &query, nil); err != nil { return err } - listIDs := make([]githubv4.ID, 0, len(repoQuery.Repository.Lists.Nodes)+1) + listIDs := make([]githubv4.ID, 0, len(query.Viewer.Lists.Nodes)+1) present := false - for _, node := range repoQuery.Repository.Lists.Nodes { - if node.ID == listID { + for _, list := range query.Viewer.Lists.Nodes { + contains := false + for _, item := range list.Items.Nodes { + if item.Repository.ID == repoID { + contains = true + break + } + } + if !contains { + continue + } + if list.ID == listID { present = true if !add { continue } } - listIDs = append(listIDs, node.ID) + listIDs = append(listIDs, list.ID) } if add && !present { listIDs = append(listIDs, listID) diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index 7139e11e2e..c37137cad1 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -121,7 +121,7 @@ func TestListUserLists(t *testing.T) { Nodes []struct { Repository struct { NameWithOwner githubv4.String - } + } `graphql:"... on Repository"` } } `graphql:"items(first: 100)"` } `graphql:"... on UserList"` @@ -135,9 +135,7 @@ func TestListUserLists(t *testing.T) { "items": map[string]any{ "nodes": []any{ map[string]any{ - "repository": map[string]any{ - "nameWithOwner": githubv4.String("owner/repo"), - }, + "nameWithOwner": githubv4.String("owner/repo"), }, }, }, @@ -577,27 +575,51 @@ func TestAddRepositoryToList(t *testing.T) { }, }), ), - // 3. read current list membership -> A, B + // 3. read current list membership by walking the viewer's lists -> A, B githubv4mock.NewQueryMatcher( struct { - Repository struct { + Viewer struct { Lists struct { Nodes []struct { - ID githubv4.ID + ID githubv4.ID + Items struct { + Nodes []struct { + Repository struct { + ID githubv4.ID + } `graphql:"... on Repository"` + } + } `graphql:"items(first: 100)"` } } `graphql:"lists(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + } }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - }, + nil, githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ + "viewer": map[string]any{ "lists": map[string]any{ "nodes": []any{ - map[string]any{"id": githubv4.ID("list-a")}, - map[string]any{"id": githubv4.ID("list-b")}, + map[string]any{ + "id": githubv4.ID("list-a"), + "items": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("repo-id")}, + }, + }, + }, + map[string]any{ + "id": githubv4.ID("list-b"), + "items": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("repo-id")}, + }, + }, + }, + map[string]any{ + "id": githubv4.ID("list-c"), + "items": map[string]any{ + "nodes": []any{}, + }, + }, }, }, }, @@ -697,24 +719,42 @@ func TestRemoveRepositoryFromList(t *testing.T) { ), githubv4mock.NewQueryMatcher( struct { - Repository struct { + Viewer struct { Lists struct { Nodes []struct { - ID githubv4.ID + ID githubv4.ID + Items struct { + Nodes []struct { + Repository struct { + ID githubv4.ID + } `graphql:"... on Repository"` + } + } `graphql:"items(first: 100)"` } } `graphql:"lists(first: 100)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + } }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - }, + nil, githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ + "viewer": map[string]any{ "lists": map[string]any{ "nodes": []any{ - map[string]any{"id": githubv4.ID("list-a")}, - map[string]any{"id": githubv4.ID("list-b")}, + map[string]any{ + "id": githubv4.ID("list-a"), + "items": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("repo-id")}, + }, + }, + }, + map[string]any{ + "id": githubv4.ID("list-b"), + "items": map[string]any{ + "nodes": []any{ + map[string]any{"id": githubv4.ID("repo-id")}, + }, + }, + }, }, }, }, From b6c27e0d253f038e9b857cc33d339c1ddeea5c38 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:16:20 -0700 Subject: [PATCH 04/12] Add end-to-end test for star list (UserList) tools Adds TestUserLists, which exercises the full lifecycle against the live GitHub API: create_user_list, update_user_list (rename), add_repository_to_list, list_user_lists (with items, to verify membership), remove_repository_from_list, and delete_user_list. The test creates its own uniquely named private list and repository, and registers cleanups for both so a failure on any step does not leave residual state behind. --- e2e/e2e_test.go | 163 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 112f653a37..420ffe3aaf 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -1997,3 +1997,166 @@ func TestPullRequestReviewDeletion(t *testing.T) { require.NoError(t, err, "expected to unmarshal text content successfully") require.Len(t, noReviews, 0, "expected to find no reviews") } + +// TestUserLists exercises the star list (UserList) tools end-to-end. It creates +// a private list, renames it, adds a repository, verifies membership, removes the +// repository, and deletes the list. Because list management is a global +// (per-account) mutation with no repository boundary, the test creates its own +// uniquely named list and cleans it up in every exit path. +func TestUserLists(t *testing.T) { + t.Parallel() + + mcpClient := setupMCPClient(t) + ctx := context.Background() + + listName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli()) + renamedList := listName + "-renamed" + + t.Cleanup(func() { + t.Logf("Cleaning up list %q...", renamedList) + resp, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "delete_user_list", + Arguments: map[string]any{"name": renamedList}, + }) + if err == nil && resp.IsError { + t.Logf("Cleanup: failed to delete list %q: %+v", renamedList, resp) + } + }) + + // Create a list. + t.Logf("Creating list %q...", listName) + resp, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "create_user_list", + Arguments: map[string]any{ + "name": listName, + "description": "e2e test list", + "is_private": true, + }, + }) + require.NoError(t, err, "expected to call 'create_user_list' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Rename the list. + t.Logf("Renaming list %q -> %q...", listName, renamedList) + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "update_user_list", + Arguments: map[string]any{ + "name": listName, + "new_name": renamedList, + }, + }) + require.NoError(t, err, "expected to call 'update_user_list' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Use the user's own account to find a repository to add. We create one so + // the test is self-contained and does not depend on any pre-existing repo. + t.Log("Getting current user...") + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{Name: "get_me"}) + require.NoError(t, err, "expected to call 'get_me' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok := resp.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + var me struct { + Login string `json:"login"` + } + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &me), "expected to unmarshal text content successfully") + currentOwner := me.Login + + repoName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli()) + t.Logf("Creating repository %s/%s...", currentOwner, repoName) + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "create_repository", + Arguments: map[string]any{ + "name": repoName, + "private": true, + "autoInit": true, + }, + }) + require.NoError(t, err, "expected to call 'create_repository' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + t.Cleanup(func() { + ghClient := getRESTClient(t) + t.Logf("Deleting repository %s/%s...", currentOwner, repoName) + _, err := ghClient.Repositories.Delete(context.Background(), currentOwner, repoName) + require.NoError(t, err, "expected to delete repository successfully") + }) + + // Add the repository to the list. + t.Logf("Adding %s/%s to list %q...", currentOwner, repoName, renamedList) + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "add_repository_to_list", + Arguments: map[string]any{ + "owner": currentOwner, + "repo": repoName, + "list_name": renamedList, + }, + }) + require.NoError(t, err, "expected to call 'add_repository_to_list' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Verify membership by listing lists with items and finding ours. + t.Log("Verifying membership via 'list_user_lists'...") + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "list_user_lists", + Arguments: map[string]any{"include_items": true}, + }) + require.NoError(t, err, "expected to call 'list_user_lists' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + textContent, ok = resp.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected content to be of type TextContent") + var listing struct { + Lists []struct { + Name string `json:"name"` + Items []struct { + Repository string `json:"repository"` + } `json:"items"` + } `json:"lists"` + } + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &listing), "expected to unmarshal text content successfully") + + var found *struct { + Name string `json:"name"` + Items []struct { + Repository string `json:"repository"` + } `json:"items"` + } + for i := range listing.Lists { + if listing.Lists[i].Name == renamedList { + found = &listing.Lists[i] + break + } + } + require.NotNil(t, found, "expected to find list %q in listing", renamedList) + var containsRepo bool + for _, item := range found.Items { + if item.Repository == currentOwner+"/"+repoName { + containsRepo = true + break + } + } + require.True(t, containsRepo, "expected list %q to contain %s/%s", renamedList, currentOwner, repoName) + + // Remove the repository from the list. + t.Logf("Removing %s/%s from list %q...", currentOwner, repoName, renamedList) + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "remove_repository_from_list", + Arguments: map[string]any{ + "owner": currentOwner, + "repo": repoName, + "list_name": renamedList, + }, + }) + require.NoError(t, err, "expected to call 'remove_repository_from_list' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + + // Delete the list (cleanup also deletes it, but this asserts the tool works). + t.Logf("Deleting list %q...", renamedList) + resp, err = mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: "delete_user_list", + Arguments: map[string]any{"name": renamedList}, + }) + require.NoError(t, err, "expected to call 'delete_user_list' tool successfully") + require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) +} From adce445e0af3cc5bc41f5757f8dec1687ced8e7f Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:37:37 -0700 Subject: [PATCH 05/12] 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. --- e2e/e2e_test.go | 11 +- .../__toolsnaps__/add_repository_to_list.snap | 2 +- .../remove_repository_from_list.snap | 4 +- pkg/github/user_lists.go | 244 +++++++++++++----- pkg/github/user_lists_test.go | 66 ++++- 5 files changed, 246 insertions(+), 81 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 420ffe3aaf..1c02aa93a5 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -2012,14 +2012,18 @@ func TestUserLists(t *testing.T) { listName := fmt.Sprintf("github-mcp-server-e2e-%s-%d", t.Name(), time.Now().UnixMilli()) renamedList := listName + "-renamed" + // currentListName tracks the list's current name so cleanup deletes the + // right one even if the rename below fails after creation. It is only + // advanced once the rename succeeds. + currentListName := listName t.Cleanup(func() { - t.Logf("Cleaning up list %q...", renamedList) + t.Logf("Cleaning up list %q...", currentListName) resp, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{ Name: "delete_user_list", - Arguments: map[string]any{"name": renamedList}, + Arguments: map[string]any{"name": currentListName}, }) if err == nil && resp.IsError { - t.Logf("Cleanup: failed to delete list %q: %+v", renamedList, resp) + t.Logf("Cleanup: failed to delete list %q: %+v", currentListName, resp) } }) @@ -2047,6 +2051,7 @@ func TestUserLists(t *testing.T) { }) require.NoError(t, err, "expected to call 'update_user_list' tool successfully") require.False(t, resp.IsError, fmt.Sprintf("expected result not to be an error: %+v", resp)) + currentListName = renamedList // Use the user's own account to find a repository to add. We create one so // the test is self-contained and does not depend on any pre-existing repo. diff --git a/pkg/github/__toolsnaps__/add_repository_to_list.snap b/pkg/github/__toolsnaps__/add_repository_to_list.snap index 31f69de696..323e0f8be0 100644 --- a/pkg/github/__toolsnaps__/add_repository_to_list.snap +++ b/pkg/github/__toolsnaps__/add_repository_to_list.snap @@ -1,7 +1,7 @@ { "annotations": { "destructiveHint": false, - "idempotentHint": false, + "idempotentHint": true, "readOnlyHint": false, "title": "Add repository to star list" }, diff --git a/pkg/github/__toolsnaps__/remove_repository_from_list.snap b/pkg/github/__toolsnaps__/remove_repository_from_list.snap index e421d5d965..33c67b3469 100644 --- a/pkg/github/__toolsnaps__/remove_repository_from_list.snap +++ b/pkg/github/__toolsnaps__/remove_repository_from_list.snap @@ -1,7 +1,7 @@ { "annotations": { - "destructiveHint": false, - "idempotentHint": false, + "destructiveHint": true, + "idempotentHint": true, "readOnlyHint": false, "title": "Remove repository from star list" }, diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 8eb3dfd7c3..126421811f 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -92,34 +92,91 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo return lists, int(query.Viewer.Lists.TotalCount), nil } -// listUserListItems returns the repositories held by a single list. +// listUserListItems returns the repositories held by a single list, following +// the items connection's cursor until every page has been consumed. func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID) ([]userListItem, error) { - var query struct { - Node struct { - UserList struct { - Items struct { - Nodes []struct { - Repository struct { - NameWithOwner githubv4.String - } `graphql:"... on Repository"` - } - } `graphql:"items(first: 100)"` - } `graphql:"... on UserList"` - } `graphql:"node(id: $id)"` - } - vars := map[string]any{ - "id": listID, - } - if err := client.Query(ctx, &query, vars); err != nil { - return nil, err - } - items := make([]userListItem, 0, len(query.Node.UserList.Items.Nodes)) - for _, node := range query.Node.UserList.Items.Nodes { - items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)}) + items := make([]userListItem, 0) + var after *githubv4.String + for { + var query struct { + Node struct { + UserList struct { + Items struct { + Nodes []struct { + Repository struct { + NameWithOwner githubv4.String + } `graphql:"... on Repository"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"items(first: 100, after: $after)"` + } `graphql:"... on UserList"` + } `graphql:"node(id: $id)"` + } + vars := map[string]any{ + "id": listID, + "after": after, + } + if err := client.Query(ctx, &query, vars); err != nil { + return nil, err + } + for _, node := range query.Node.UserList.Items.Nodes { + items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)}) + } + if !query.Node.UserList.Items.PageInfo.HasNextPage { + break + } + cursor := githubv4.String(query.Node.UserList.Items.PageInfo.EndCursor) + after = &cursor } return items, nil } +// repoInList reports whether the repository identified by repoID belongs to the +// list identified by listID, paging through the list's items until a match is +// found or the connection is exhausted. +func repoInList(ctx context.Context, client *githubv4.Client, listID, repoID githubv4.ID) (bool, error) { + var after *githubv4.String + for { + var query struct { + Node struct { + UserList struct { + Items struct { + Nodes []struct { + Repository struct { + ID githubv4.ID + } `graphql:"... on Repository"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"items(first: 100, after: $after)"` + } `graphql:"... on UserList"` + } `graphql:"node(id: $id)"` + } + vars := map[string]any{ + "id": listID, + "after": after, + } + if err := client.Query(ctx, &query, vars); err != nil { + return false, err + } + for _, node := range query.Node.UserList.Items.Nodes { + if node.Repository.ID == repoID { + return true, nil + } + } + if !query.Node.UserList.Items.PageInfo.HasNextPage { + return false, nil + } + cursor := githubv4.String(query.Node.UserList.Items.PageInfo.EndCursor) + after = &cursor + } +} + // createUserList creates a new star list for the authenticated user. func createUserList(ctx context.Context, client *githubv4.Client, name, description string, isPrivate *bool) (string, error) { input := githubv4.CreateUserListInput{ @@ -150,8 +207,10 @@ func createUserList(ctx context.Context, client *githubv4.Client, name, descript // updateUserList updates the name, description, and/or privacy of an existing // star list. name identifies the list; newName, description, and isPrivate are -// optional changes. -func updateUserList(ctx context.Context, client *githubv4.Client, name, newName, description string, isPrivate *bool) (string, error) { +// optional changes. description is nil when the field was omitted (leave +// unchanged) and non-nil when supplied (including an explicit empty string, +// which clears the description). +func updateUserList(ctx context.Context, client *githubv4.Client, name, newName string, description *string, isPrivate *bool) (string, error) { listID, err := getUserListID(ctx, client, name) if err != nil { return "", err @@ -164,8 +223,8 @@ func updateUserList(ctx context.Context, client *githubv4.Client, name, newName, n := githubv4.String(newName) input.Name = &n } - if description != "" { - d := githubv4.String(description) + if description != nil { + d := githubv4.String(*description) input.Description = &d } if isPrivate != nil { @@ -215,7 +274,9 @@ func deleteUserList(ctx context.Context, client *githubv4.Client, name string) e // GitHub's schema has no reverse lookup from a repository to its lists (there // is no `lists` field on Repository). Membership is instead derived by walking // the viewer's lists and checking each list's items for the repository's node -// ID. +// ID. Both the lists and each list's items are fully paginated so a repository +// beyond the first 100 items of a list is still counted; omitting it here would +// silently drop the repository from that list on the subsequent mutation. func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, repo, listName string, add bool) error { listID, err := getUserListID(ctx, client, listName) if err != nil { @@ -227,54 +288,97 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, return fmt.Errorf("failed to find repository: %w", err) } - var query struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Items struct { - Nodes []struct { - Repository struct { - ID githubv4.ID - } `graphql:"... on Repository"` - } - } `graphql:"items(first: 100)"` + // Walk every list and, for each, every page of items to determine which + // lists currently contain the repository. + var listIDs []githubv4.ID + var listsAfter *githubv4.String + for { + var query struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Items struct { + Nodes []struct { + Repository struct { + ID githubv4.ID + } `graphql:"... on Repository"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"items(first: 100)"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"lists(first: 100, after: $listsAfter)"` + } + } + vars := map[string]any{ + "listsAfter": listsAfter, + } + if err := client.Query(ctx, &query, vars); err != nil { + return err + } + for _, list := range query.Viewer.Lists.Nodes { + contains := false + for _, item := range list.Items.Nodes { + if item.Repository.ID == repoID { + contains = true + break } - } `graphql:"lists(first: 100)"` + } + // If the first page didn't contain the repository but the list has + // more than 100 items, keep paging until we know for certain. + if !contains && list.Items.PageInfo.HasNextPage { + var err error + contains, err = repoInList(ctx, client, list.ID, repoID) + if err != nil { + return err + } + } + if contains { + listIDs = append(listIDs, list.ID) + } } - } - if err := client.Query(ctx, &query, nil); err != nil { - return err + + if !query.Viewer.Lists.PageInfo.HasNextPage { + break + } + cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) + listsAfter = &cursor } - listIDs := make([]githubv4.ID, 0, len(query.Viewer.Lists.Nodes)+1) present := false - for _, list := range query.Viewer.Lists.Nodes { - contains := false - for _, item := range list.Items.Nodes { - if item.Repository.ID == repoID { - contains = true - break - } + for _, id := range listIDs { + if id == listID { + present = true + break } - if !contains { - continue + } + + result := make([]githubv4.ID, 0, len(listIDs)+1) + if add { + for _, id := range listIDs { + result = append(result, id) } - if list.ID == listID { - present = true - if !add { - continue + if !present { + result = append(result, listID) + } + } else { + for _, id := range listIDs { + if id != listID { + result = append(result, id) } } - listIDs = append(listIDs, list.ID) - } - if add && !present { - listIDs = append(listIDs, listID) } input := githubv4.UpdateUserListsForItemInput{ ItemID: repoID, - ListIDs: listIDs, + ListIDs: result, } var mutation struct { UpdateUserListsForItem struct { @@ -447,7 +551,7 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - description, err := OptionalParam[string](args, "description") + description, descPresent, err := OptionalParamOK[string](args, "description") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -456,7 +560,7 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(err.Error()), nil, nil } - if newName == "" && description == "" && !present { + if newName == "" && !descPresent && !present { return utils.NewToolResultError("at least one of new_name, description, or is_private must be provided for update"), nil, nil } @@ -469,7 +573,11 @@ func UpdateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { if present { isPrivatePtr = &isPrivate } - updatedName, err := updateUserList(ctx, client, name, newName, description, isPrivatePtr) + var descriptionPtr *string + if descPresent { + descriptionPtr = &description + } + updatedName, err := updateUserList(ctx, client, name, newName, descriptionPtr, isPrivatePtr) if err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to update user list", err), nil, nil } @@ -532,6 +640,7 @@ func AddRepositoryToList(t translations.TranslationHelperFunc) inventory.ServerT Title: t("TOOL_ADD_REPOSITORY_TO_LIST_USER_TITLE", "Add repository to star list"), ReadOnlyHint: false, DestructiveHint: jsonschema.Ptr(false), + IdempotentHint: true, }, InputSchema: &jsonschema.Schema{ Type: "object", @@ -590,7 +699,8 @@ func RemoveRepositoryFromList(t translations.TranslationHelperFunc) inventory.Se Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_REMOVE_REPOSITORY_FROM_LIST_USER_TITLE", "Remove repository from star list"), ReadOnlyHint: false, - DestructiveHint: jsonschema.Ptr(false), + DestructiveHint: jsonschema.Ptr(true), + IdempotentHint: true, }, InputSchema: &jsonschema.Schema{ Type: "object", diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index c37137cad1..26761a784c 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -123,12 +123,17 @@ func TestListUserLists(t *testing.T) { NameWithOwner githubv4.String } `graphql:"... on Repository"` } - } `graphql:"items(first: 100)"` + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"items(first: 100, after: $after)"` } `graphql:"... on UserList"` } `graphql:"node(id: $id)"` }{}, map[string]any{ - "id": githubv4.ID("list-1"), + "id": githubv4.ID("list-1"), + "after": (*githubv4.String)(nil), }, githubv4mock.DataResponse(map[string]any{ "node": map[string]any{ @@ -138,6 +143,10 @@ func TestListUserLists(t *testing.T) { "nameWithOwner": githubv4.String("owner/repo"), }, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, }), @@ -530,6 +539,7 @@ func TestAddRepositoryToList(t *testing.T) { assert.False(t, tool.Annotations.ReadOnlyHint) require.NotNil(t, tool.Annotations.DestructiveHint) assert.False(t, *tool.Annotations.DestructiveHint) + assert.True(t, tool.Annotations.IdempotentHint) assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) // Repository currently in lists "A" and "B"; adding to "C" must resubmit all @@ -588,12 +598,22 @@ func TestAddRepositoryToList(t *testing.T) { ID githubv4.ID } `graphql:"... on Repository"` } + PageInfo struct { + HasNextPage bool + EndCursor string + } } `graphql:"items(first: 100)"` } - } `graphql:"lists(first: 100)"` + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"lists(first: 100, after: $listsAfter)"` } }{}, - nil, + map[string]any{ + "listsAfter": (*githubv4.String)(nil), + }, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -604,6 +624,10 @@ func TestAddRepositoryToList(t *testing.T) { "nodes": []any{ map[string]any{"id": githubv4.ID("repo-id")}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, map[string]any{ @@ -612,15 +636,24 @@ func TestAddRepositoryToList(t *testing.T) { "nodes": []any{ map[string]any{"id": githubv4.ID("repo-id")}, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, map[string]any{ "id": githubv4.ID("list-c"), "items": map[string]any{ - "nodes": []any{}, + "nodes": []any{}, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, }, }, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, }), @@ -673,7 +706,8 @@ func TestRemoveRepositoryFromList(t *testing.T) { assert.Equal(t, "remove_repository_from_list", tool.Name) assert.False(t, tool.Annotations.ReadOnlyHint) require.NotNil(t, tool.Annotations.DestructiveHint) - assert.False(t, *tool.Annotations.DestructiveHint) + assert.True(t, *tool.Annotations.DestructiveHint) + assert.True(t, tool.Annotations.IdempotentHint) assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) // Repository currently in lists "A" and "B"; removing from "B" must resubmit @@ -729,12 +763,22 @@ func TestRemoveRepositoryFromList(t *testing.T) { ID githubv4.ID } `graphql:"... on Repository"` } + PageInfo struct { + HasNextPage bool + EndCursor string + } } `graphql:"items(first: 100)"` } - } `graphql:"lists(first: 100)"` + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"lists(first: 100, after: $listsAfter)"` } }{}, - nil, + map[string]any{ + "listsAfter": (*githubv4.String)(nil), + }, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -745,6 +789,7 @@ func TestRemoveRepositoryFromList(t *testing.T) { "nodes": []any{ map[string]any{"id": githubv4.ID("repo-id")}, }, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, }, }, map[string]any{ @@ -753,9 +798,14 @@ func TestRemoveRepositoryFromList(t *testing.T) { "nodes": []any{ map[string]any{"id": githubv4.ID("repo-id")}, }, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, }, }, }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "", + }, }, }, }), From 89120cafa2f58e8be71b3a640999552e79c63031 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:12:25 -0700 Subject: [PATCH 06/12] Prevent lost updates in user list membership changes Serialize replacement-style membership updates per repository within the server process, skip already-satisfied add/remove operations, and add regression coverage for lock serialization, no-op writes, and memberships discovered beyond the first items page. --- pkg/github/user_lists.go | 50 +++++++++++++++ pkg/github/user_lists_test.go | 111 ++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 126421811f..862584804d 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "strings" + "sync" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/inventory" @@ -28,6 +30,48 @@ type userListItem struct { Repository string `json:"repository"` } +type refCountedMutex struct { + mutex sync.Mutex + refs int +} + +var repoListMembershipLocks = struct { + sync.Mutex + locks map[string]*refCountedMutex +}{ + locks: make(map[string]*refCountedMutex), +} + +// lockRepoListMembership serializes replacement-style membership updates for +// the same repository within this process. The lock is repository-wide rather +// than account-specific because ToolDependencies intentionally does not expose +// credential identity; this is a stronger serialization boundary and avoids +// cross-account lost updates inside a shared server process. +func lockRepoListMembership(owner, repo string) func() { + key := strings.ToLower(owner + "/" + repo) + + repoListMembershipLocks.Lock() + lock := repoListMembershipLocks.locks[key] + if lock == nil { + lock = &refCountedMutex{} + repoListMembershipLocks.locks[key] = lock + } + lock.refs++ + repoListMembershipLocks.Unlock() + + lock.mutex.Lock() + return func() { + lock.mutex.Unlock() + + repoListMembershipLocks.Lock() + lock.refs-- + if lock.refs == 0 { + delete(repoListMembershipLocks.locks, key) + } + repoListMembershipLocks.Unlock() + } +} + // getUserListID resolves the authenticated user's list with the given name to // its node ID. It returns an error when no list matches the name. func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { @@ -288,6 +332,9 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, return fmt.Errorf("failed to find repository: %w", err) } + unlock := lockRepoListMembership(owner, repo) + defer unlock() + // Walk every list and, for each, every page of items to determine which // lists currently contain the repository. var listIDs []githubv4.ID @@ -359,6 +406,9 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, break } } + if add == present { + return nil + } result := make([]githubv4.ID, 0, len(listIDs)+1) if add { diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index 26761a784c..3c70498920 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "testing" + "time" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" @@ -893,3 +894,113 @@ func TestAddRepositoryToListListNotFound(t *testing.T) { textContent := getErrorResult(t, result) assert.Contains(t, textContent.Text, "list 'Missing' not found") } + +func TestRepoListMembershipLockSerializesConcurrentUpdates(t *testing.T) { + unlockFirst := lockRepoListMembership("ConcurrentOwner", "ConcurrentRepo") + firstReleased := false + defer func() { + if !firstReleased { + unlockFirst() + } + }() + + started := make(chan struct{}) + acquired := make(chan struct{}) + done := make(chan struct{}) + go func() { + close(started) + unlockSecond := lockRepoListMembership("concurrentowner", "concurrentrepo") + close(acquired) + unlockSecond() + close(done) + }() + <-started + + select { + case <-acquired: + t.Fatal("second update acquired the same repository lock before the first released it") + case <-time.After(25 * time.Millisecond): + } + + unlockFirst() + firstReleased = true + select { + case <-acquired: + case <-time.After(time.Second): + t.Fatal("second update did not acquire the repository lock after release") + } + <-done +} + +func TestSetRepoListMembershipsSkipsSatisfiedMutation(t *testing.T) { + tests := []struct { + name string + add bool + currentlyInIt bool + }{ + {name: "add already present", add: true, currentlyInIt: true}, + {name: "remove already absent", add: false, currentlyInIt: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + itemNodes := "[]" + if tc.currentlyInIt { + itemNodes = `[{"id":"repo-id"}]` + } + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-c","name":"C"}]}}}}` + }, + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"repository":{"id":"repo-id"}}}` + }, + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-c","items":{"nodes":` + itemNodes + `,"pageInfo":{"hasNextPage":false,"endCursor":""}}}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` + }, + }, + } + client := githubv4.NewClient(&http.Client{Transport: transport}) + + require.NoError(t, setRepoListMemberships(context.Background(), client, "owner", "repo", "C", tc.add)) + require.Len(t, transport.calls, 3, "satisfied operation should not issue a mutation") + }) + } +} + +func TestSetRepoListMembershipsPreservesMembershipFoundOnLaterPage(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-c","name":"C"}]}}}}` + }, + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"repository":{"id":"repo-id"}}}` + }, + func(capturedGraphQLRequest) (int, string) { + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-a","items":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-a"}}},{"id":"list-c","items":{"nodes":[],"pageInfo":{"hasNextPage":false,"endCursor":""}}}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + assert.Nil(t, req.Variables["after"]) + return http.StatusOK, `{"data":{"node":{"items":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-a"}}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + assert.Equal(t, "cursor-a", req.Variables["after"]) + return http.StatusOK, `{"data":{"node":{"items":{"nodes":[{"id":"repo-id"}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + input, ok := req.Variables["input"].(map[string]any) + require.True(t, ok) + assert.Equal(t, []any{"list-a", "list-c"}, input["listIds"]) + return http.StatusOK, `{"data":{"updateUserListsForItem":{"clientMutationId":"test-mutation-id"}}}` + }, + }, + } + client := githubv4.NewClient(&http.Client{Transport: transport}) + + require.NoError(t, setRepoListMemberships(context.Background(), client, "owner", "repo", "C", true)) + require.Len(t, transport.calls, 6) +} From 47f57f23ffef5d88eeb169ccd40e766a2123f3e7 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:53:40 -0700 Subject: [PATCH 07/12] Address latest user list review feedback Label user-list results as private trusted when IFC labels are enabled and continue membership pagination from the cursor already fetched. Remove the process-local membership lock and its serialization test, while retaining no-op and later-page regression coverage. --- pkg/github/user_lists.go | 58 +++------------------- pkg/github/user_lists_test.go | 92 +++++++++++++++++++---------------- pkg/ifc/ifc.go | 9 ++++ pkg/ifc/ifc_test.go | 6 +++ 4 files changed, 71 insertions(+), 94 deletions(-) diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 862584804d..1d7b9cd716 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -4,10 +4,9 @@ import ( "context" "encoding/json" "fmt" - "strings" - "sync" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/scopes" "github.com/github/github-mcp-server/pkg/translations" @@ -30,48 +29,6 @@ type userListItem struct { Repository string `json:"repository"` } -type refCountedMutex struct { - mutex sync.Mutex - refs int -} - -var repoListMembershipLocks = struct { - sync.Mutex - locks map[string]*refCountedMutex -}{ - locks: make(map[string]*refCountedMutex), -} - -// lockRepoListMembership serializes replacement-style membership updates for -// the same repository within this process. The lock is repository-wide rather -// than account-specific because ToolDependencies intentionally does not expose -// credential identity; this is a stronger serialization boundary and avoids -// cross-account lost updates inside a shared server process. -func lockRepoListMembership(owner, repo string) func() { - key := strings.ToLower(owner + "/" + repo) - - repoListMembershipLocks.Lock() - lock := repoListMembershipLocks.locks[key] - if lock == nil { - lock = &refCountedMutex{} - repoListMembershipLocks.locks[key] = lock - } - lock.refs++ - repoListMembershipLocks.Unlock() - - lock.mutex.Lock() - return func() { - lock.mutex.Unlock() - - repoListMembershipLocks.Lock() - lock.refs-- - if lock.refs == 0 { - delete(repoListMembershipLocks.locks, key) - } - repoListMembershipLocks.Unlock() - } -} - // getUserListID resolves the authenticated user's list with the given name to // its node ID. It returns an error when no list matches the name. func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { @@ -181,8 +138,7 @@ func listUserListItems(ctx context.Context, client *githubv4.Client, listID gith // repoInList reports whether the repository identified by repoID belongs to the // list identified by listID, paging through the list's items until a match is // found or the connection is exhausted. -func repoInList(ctx context.Context, client *githubv4.Client, listID, repoID githubv4.ID) (bool, error) { - var after *githubv4.String +func repoInList(ctx context.Context, client *githubv4.Client, listID, repoID githubv4.ID, after *githubv4.String) (bool, error) { for { var query struct { Node struct { @@ -332,9 +288,6 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, return fmt.Errorf("failed to find repository: %w", err) } - unlock := lockRepoListMembership(owner, repo) - defer unlock() - // Walk every list and, for each, every page of items to determine which // lists currently contain the repository. var listIDs []githubv4.ID @@ -381,8 +334,9 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, // If the first page didn't contain the repository but the list has // more than 100 items, keep paging until we know for certain. if !contains && list.Items.PageInfo.HasNextPage { + cursor := githubv4.String(list.Items.PageInfo.EndCursor) var err error - contains, err = repoInList(ctx, client, list.ID, repoID) + contains, err = repoInList(ctx, client, list.ID, repoID, &cursor) if err != nil { return err } @@ -487,7 +441,9 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return nil, nil, fmt.Errorf("failed to marshal user lists: %w", err) } - return utils.NewToolResultText(string(out)), nil, nil + result := utils.NewToolResultText(string(out)) + result = attachStaticIFCLabel(ctx, deps, result, ifc.LabelUserList()) + return result, nil, nil }, ) } diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index 3c70498920..8ddabde705 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -4,10 +4,10 @@ import ( "context" "net/http" "testing" - "time" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/translations" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" @@ -179,6 +179,53 @@ func TestListUserLists(t *testing.T) { } } +func TestListUserListsIFCLabel(t *testing.T) { + t.Parallel() + + serverTool := ListUserLists(translations.NullTranslationHelper) + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100)"` + } + }{}, + nil, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{}, + "totalCount": githubv4.Int(0), + }, + }, + }), + ), + ) + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{ + GQLClient: client, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), + } + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{}) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + label, ok := result.Meta["ifc"].(ifc.SecurityLabel) + require.True(t, ok) + assert.Equal(t, ifc.PrivateTrusted(), label) +} + func TestCreateUserList(t *testing.T) { t.Parallel() @@ -895,43 +942,6 @@ func TestAddRepositoryToListListNotFound(t *testing.T) { assert.Contains(t, textContent.Text, "list 'Missing' not found") } -func TestRepoListMembershipLockSerializesConcurrentUpdates(t *testing.T) { - unlockFirst := lockRepoListMembership("ConcurrentOwner", "ConcurrentRepo") - firstReleased := false - defer func() { - if !firstReleased { - unlockFirst() - } - }() - - started := make(chan struct{}) - acquired := make(chan struct{}) - done := make(chan struct{}) - go func() { - close(started) - unlockSecond := lockRepoListMembership("concurrentowner", "concurrentrepo") - close(acquired) - unlockSecond() - close(done) - }() - <-started - - select { - case <-acquired: - t.Fatal("second update acquired the same repository lock before the first released it") - case <-time.After(25 * time.Millisecond): - } - - unlockFirst() - firstReleased = true - select { - case <-acquired: - case <-time.After(time.Second): - t.Fatal("second update did not acquire the repository lock after release") - } - <-done -} - func TestSetRepoListMembershipsSkipsSatisfiedMutation(t *testing.T) { tests := []struct { name string @@ -983,10 +993,6 @@ func TestSetRepoListMembershipsPreservesMembershipFoundOnLaterPage(t *testing.T) func(capturedGraphQLRequest) (int, string) { return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-a","items":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-a"}}},{"id":"list-c","items":{"nodes":[],"pageInfo":{"hasNextPage":false,"endCursor":""}}}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` }, - func(req capturedGraphQLRequest) (int, string) { - assert.Nil(t, req.Variables["after"]) - return http.StatusOK, `{"data":{"node":{"items":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-a"}}}}}` - }, func(req capturedGraphQLRequest) (int, string) { assert.Equal(t, "cursor-a", req.Variables["after"]) return http.StatusOK, `{"data":{"node":{"items":{"nodes":[{"id":"repo-id"}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` @@ -1002,5 +1008,5 @@ func TestSetRepoListMembershipsPreservesMembershipFoundOnLaterPage(t *testing.T) client := githubv4.NewClient(&http.Client{Transport: transport}) require.NoError(t, setRepoListMemberships(context.Background(), client, "owner", "repo", "C", true)) - require.Len(t, transport.calls, 6) + require.Len(t, transport.calls, 5) } diff --git a/pkg/ifc/ifc.go b/pkg/ifc/ifc.go index f23383ce73..0b62c67d56 100644 --- a/pkg/ifc/ifc.go +++ b/pkg/ifc/ifc.go @@ -73,6 +73,15 @@ func LabelGetMe() SecurityLabel { return PrivateTrusted() } +// LabelUserList returns the IFC label for the authenticated user's star lists. +// List names and descriptions may be private, and included items may expose +// memberships in private repositories, so the joined result is conservatively +// private. Integrity is trusted because the lists are maintained by the +// authenticated user through GitHub. +func LabelUserList() SecurityLabel { + return PrivateTrusted() +} + // LabelListIssues returns the IFC label for a list_issues result. // Public repositories are universally readable; private repositories are // restricted to their collaborators (resolved client-side from the marker). diff --git a/pkg/ifc/ifc_test.go b/pkg/ifc/ifc_test.go index f4b25c1876..5b7d1fdb4a 100644 --- a/pkg/ifc/ifc_test.go +++ b/pkg/ifc/ifc_test.go @@ -121,6 +121,12 @@ func TestLabelGetMe(t *testing.T) { assert.Equal(t, ConfidentialityPrivate, label.Confidentiality) } +func TestLabelUserList(t *testing.T) { + t.Parallel() + + assert.Equal(t, PrivateTrusted(), LabelUserList()) +} + func TestLabelRelease(t *testing.T) { t.Parallel() From ad995550e1b284b7b20105901c3f6790ae266728 Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:35:31 -0700 Subject: [PATCH 08/12] Paginate user lists and challenge for item scopes Fully paginate the viewer's UserList connection for both listing and name resolution. Make list_user_lists use a dynamic OAuth challenge so metadata requires read:user while include_items also requires repo, and add pagination and scope regression coverage. --- README.md | 2 +- pkg/github/tool_scopes.go | 18 +++ pkg/github/tool_scopes_test.go | 1 + pkg/github/user_lists.go | 125 +++++++++++++-------- pkg/github/user_lists_test.go | 196 +++++++++++++-------------------- 5 files changed, 173 insertions(+), 169 deletions(-) diff --git a/README.md b/README.md index be7e59202e..e0f022b820 100644 --- a/README.md +++ b/README.md @@ -1506,7 +1506,7 @@ The following sets of tools are available: - `username`: Username to list starred repositories for. Defaults to the authenticated user. (string, optional) - **list_user_lists** - List star lists - - **OAuth Challenge Scopes**: `read:user` + - **OAuth Challenge Scopes**: `read:user`, `repo` - `include_items`: Whether to include the repositories in each list. (boolean, optional) - **remove_repository_from_list** - Remove repository from star list diff --git a/pkg/github/tool_scopes.go b/pkg/github/tool_scopes.go index 4482a6e243..480ddd947a 100644 --- a/pkg/github/tool_scopes.go +++ b/pkg/github/tool_scopes.go @@ -69,3 +69,21 @@ func uiGetScopeAccess() inventory.ScopeAccess { }, ) } + +func userListReadScopeAccess() inventory.ScopeAccess { + return scopes.DynamicChallenge( + []scopes.Scope{scopes.ReadUser, scopes.Repo}, + func([]string) bool { + // User-list metadata may be readable with read:user, while MCP OAuth + // can challenge for missing scopes at call time. + return true + }, + func(arguments map[string]any, activeScopes []string) []string { + includeItems, ok := arguments["include_items"].(bool) + if ok && includeItems { + return scopes.ChallengeAll(activeScopes, scopes.ReadUser, scopes.Repo) + } + return scopes.ChallengeAll(activeScopes, scopes.ReadUser) + }, + ) +} diff --git a/pkg/github/tool_scopes_test.go b/pkg/github/tool_scopes_test.go index 6854237f56..04928555d5 100644 --- a/pkg/github/tool_scopes_test.go +++ b/pkg/github/tool_scopes_test.go @@ -109,6 +109,7 @@ func TestDynamicToolScopeMetadataIsExhaustive(t *testing.T) { {tool: ListIssueFields(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}}, {tool: ListIssueTypes(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}}, {tool: UIGet(translations.NullTranslationHelper), maxScopes: []string{"repo", "read:org"}}, + {tool: ListUserLists(translations.NullTranslationHelper), maxScopes: []string{"read:user", "repo"}}, } for _, tt := range tests { diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 1d7b9cd716..3594df6715 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -29,26 +29,59 @@ type userListItem struct { Repository string `json:"repository"` } +type userListLookupQuery struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"lists(first: 100, after: $after)"` + } +} + +type userListPageQuery struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100, after: $after)"` + } +} + // getUserListID resolves the authenticated user's list with the given name to // its node ID. It returns an error when no list matches the name. func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { - var query struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` + var after *githubv4.String + for { + var query userListLookupQuery + vars := map[string]any{"after": after} + if err := client.Query(ctx, &query, vars); err != nil { + return "", err } - } - if err := client.Query(ctx, &query, nil); err != nil { - return "", err - } - for _, node := range query.Viewer.Lists.Nodes { - if string(node.Name) == name { - return node.ID, nil + for _, node := range query.Viewer.Lists.Nodes { + if string(node.Name) == name { + return node.ID, nil + } } + if !query.Viewer.Lists.PageInfo.HasNextPage { + break + } + cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) + after = &cursor } return "", fmt.Errorf("list '%s' not found", name) } @@ -56,41 +89,39 @@ func getUserListID(ctx context.Context, client *githubv4.Client, name string) (g // listUserLists returns the authenticated user's star lists, optionally // including the repositories each list contains. func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bool) ([]userList, int, error) { - var query struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - IsPrivate githubv4.Boolean - } - TotalCount githubv4.Int - } `graphql:"lists(first: 100)"` - } - } - if err := client.Query(ctx, &query, nil); err != nil { - return nil, 0, err - } - - lists := make([]userList, 0, len(query.Viewer.Lists.Nodes)) - for _, node := range query.Viewer.Lists.Nodes { - list := userList{ - ID: node.ID, - Name: string(node.Name), - Description: string(node.Description), - IsPrivate: bool(node.IsPrivate), + lists := make([]userList, 0) + totalCount := 0 + var after *githubv4.String + for { + var query userListPageQuery + vars := map[string]any{"after": after} + if err := client.Query(ctx, &query, vars); err != nil { + return nil, 0, err } - if includeItems { - items, err := listUserListItems(ctx, client, node.ID) - if err != nil { - return nil, 0, err + totalCount = int(query.Viewer.Lists.TotalCount) + for _, node := range query.Viewer.Lists.Nodes { + list := userList{ + ID: node.ID, + Name: string(node.Name), + Description: string(node.Description), + IsPrivate: bool(node.IsPrivate), + } + if includeItems { + items, err := listUserListItems(ctx, client, node.ID) + if err != nil { + return nil, 0, err + } + list.Items = items } - list.Items = items + lists = append(lists, list) } - lists = append(lists, list) + if !query.Viewer.Lists.PageInfo.HasNextPage { + break + } + cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) + after = &cursor } - return lists, int(query.Viewer.Lists.TotalCount), nil + return lists, totalCount, nil } // listUserListItems returns the repositories held by a single list, following @@ -416,7 +447,7 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { }, }, }, - scopes.PublicRead(scopes.ReadUser), + userListReadScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { includeItems, err := OptionalParam[bool](args, "include_items") if err != nil { diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index 8ddabde705..b8fd6c378d 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -25,11 +25,17 @@ func TestListUserLists(t *testing.T) { assert.NotEmpty(t, tool.Description) assert.True(t, tool.Annotations.ReadOnlyHint, "list_user_lists tool should be read-only") - // scope gating: PublicRead(ReadUser) - assert.Equal(t, []string{"read:user"}, serverTool.ScopeAccess.Scopes) + // Scope gating: list metadata needs read:user, while include_items also + // requires repo so private repository memberships are not silently omitted. + assert.Equal(t, []string{"read:user", "repo"}, serverTool.ScopeAccess.Scopes) assert.NotNil(t, serverTool.ScopeAccess.Visible) assert.NotNil(t, serverTool.ScopeAccess.Challenge) + assert.True(t, serverTool.ScopeAccess.Dynamic) assert.True(t, serverTool.ScopeAccess.Visible(nil)) + assert.Empty(t, serverTool.ScopeAccess.Challenge(map[string]any{}, []string{"read:user"})) + assert.ElementsMatch(t, []string{"read:user", "repo"}, serverTool.ScopeAccess.Challenge( + map[string]any{"include_items": true}, []string{"read:user"}, + )) tests := []struct { name string @@ -44,20 +50,8 @@ func TestListUserLists(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - IsPrivate githubv4.Boolean - } - TotalCount githubv4.Int - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListPageQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -84,20 +78,8 @@ func TestListUserLists(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - IsPrivate githubv4.Boolean - } - TotalCount githubv4.Int - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListPageQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -185,20 +167,8 @@ func TestListUserListsIFCLabel(t *testing.T) { serverTool := ListUserLists(translations.NullTranslationHelper) mockedClient := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - Description githubv4.String - IsPrivate githubv4.Boolean - } - TotalCount githubv4.Int - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListPageQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -226,6 +196,53 @@ func TestListUserListsIFCLabel(t *testing.T) { assert.Equal(t, ifc.PrivateTrusted(), label) } +func TestGetUserListIDPaginates(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + assert.Nil(t, req.Variables["after"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-1","name":"Other"}],"pageInfo":{"hasNextPage":true,"endCursor":"next-list"}}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + assert.Equal(t, "next-list", req.Variables["after"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-2","name":"Target"}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` + }, + }, + } + client := githubv4.NewClient(&http.Client{Transport: transport}) + + id, err := getUserListID(context.Background(), client, "Target") + require.NoError(t, err) + assert.Equal(t, githubv4.ID("list-2"), id) + require.Len(t, transport.calls, 2) +} + +func TestListUserListsPaginates(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + assert.Nil(t, req.Variables["after"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-1","name":"First","description":"","isPrivate":false}],"pageInfo":{"hasNextPage":true,"endCursor":"next-list"},"totalCount":2}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + assert.Equal(t, "next-list", req.Variables["after"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-2","name":"Second","description":"","isPrivate":true}],"pageInfo":{"hasNextPage":false,"endCursor":""},"totalCount":2}}}}` + }, + }, + } + client := githubv4.NewClient(&http.Client{Transport: transport}) + + lists, totalCount, err := listUserLists(context.Background(), client, false) + require.NoError(t, err) + require.Len(t, lists, 2) + assert.Equal(t, "First", lists[0].Name) + assert.Equal(t, "Second", lists[1].Name) + assert.Equal(t, 2, totalCount) + require.Len(t, transport.calls, 2) +} + func TestCreateUserList(t *testing.T) { t.Parallel() @@ -332,17 +349,8 @@ func TestUpdateUserList(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -388,17 +396,8 @@ func TestUpdateUserList(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -475,17 +474,8 @@ func TestDeleteUserList(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -525,17 +515,8 @@ func TestDeleteUserList(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -595,17 +576,8 @@ func TestAddRepositoryToList(t *testing.T) { mockedClient := githubv4mock.NewMockedHTTPClient( // 1. resolve list "C" -> list-c githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -762,17 +734,8 @@ func TestRemoveRepositoryFromList(t *testing.T) { // only the remainder ("A"). mockedClient := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -901,17 +864,8 @@ func TestAddRepositoryToListListNotFound(t *testing.T) { serverTool := AddRepositoryToList(translations.NullTranslationHelper) mockedClient := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Lists struct { - Nodes []struct { - ID githubv4.ID - Name githubv4.String - } - } `graphql:"lists(first: 100)"` - } - }{}, - nil, + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ From ab5f0110ce6d4b30826b28fd54433c202206cdde Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:56:17 -0700 Subject: [PATCH 09/12] Protect private membership reads and reduce list queries Require repo scope for repository list membership mutations so private memberships are preserved. Fetch each list's first item page inline when include_items is requested, then continue only lists with additional pages. Update focused tests and generated scope documentation. --- README.md | 4 +- pkg/github/user_lists.go | 79 ++++++++++++++++++++++++++++++----- pkg/github/user_lists_test.go | 76 ++++++++++++++++----------------- 3 files changed, 105 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index e0f022b820..c397d2fa38 100644 --- a/README.md +++ b/README.md @@ -1482,7 +1482,7 @@ The following sets of tools are available: star Stargazers - **add_repository_to_list** - Add repository to star list - - **OAuth Challenge Scopes**: `user` + - **OAuth Challenge Scopes**: `user`, `repo` - `list_name`: The name of the star list to add the repository to. (string, required) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) @@ -1510,7 +1510,7 @@ The following sets of tools are available: - `include_items`: Whether to include the repositories in each list. (boolean, optional) - **remove_repository_from_list** - Remove repository from star list - - **OAuth Challenge Scopes**: `user` + - **OAuth Challenge Scopes**: `user`, `repo` - `list_name`: The name of the star list to remove the repository from. (string, required) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 3594df6715..938333c256 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -62,6 +62,35 @@ type userListPageQuery struct { } } +type userListPageWithItemsQuery struct { + Viewer struct { + Lists struct { + Nodes []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + IsPrivate githubv4.Boolean + Items struct { + Nodes []struct { + Repository struct { + NameWithOwner githubv4.String + } `graphql:"... on Repository"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } `graphql:"items(first: 100)"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + TotalCount githubv4.Int + } `graphql:"lists(first: 100, after: $after)"` + } +} + // getUserListID resolves the authenticated user's list with the given name to // its node ID. It returns an error when no list matches the name. func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { @@ -93,6 +122,42 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo totalCount := 0 var after *githubv4.String for { + if includeItems { + var query userListPageWithItemsQuery + vars := map[string]any{"after": after} + if err := client.Query(ctx, &query, vars); err != nil { + return nil, 0, err + } + totalCount = int(query.Viewer.Lists.TotalCount) + for _, node := range query.Viewer.Lists.Nodes { + items := make([]userListItem, 0, len(node.Items.Nodes)) + for _, item := range node.Items.Nodes { + items = append(items, userListItem{Repository: string(item.Repository.NameWithOwner)}) + } + if node.Items.PageInfo.HasNextPage { + cursor := githubv4.String(node.Items.PageInfo.EndCursor) + remaining, err := listUserListItems(ctx, client, node.ID, &cursor) + if err != nil { + return nil, 0, err + } + items = append(items, remaining...) + } + lists = append(lists, userList{ + ID: node.ID, + Name: string(node.Name), + Description: string(node.Description), + IsPrivate: bool(node.IsPrivate), + Items: items, + }) + } + if !query.Viewer.Lists.PageInfo.HasNextPage { + break + } + cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) + after = &cursor + continue + } + var query userListPageQuery vars := map[string]any{"after": after} if err := client.Query(ctx, &query, vars); err != nil { @@ -106,13 +171,6 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo Description: string(node.Description), IsPrivate: bool(node.IsPrivate), } - if includeItems { - items, err := listUserListItems(ctx, client, node.ID) - if err != nil { - return nil, 0, err - } - list.Items = items - } lists = append(lists, list) } if !query.Viewer.Lists.PageInfo.HasNextPage { @@ -126,9 +184,8 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo // listUserListItems returns the repositories held by a single list, following // the items connection's cursor until every page has been consumed. -func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID) ([]userListItem, error) { +func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID, after *githubv4.String) ([]userListItem, error) { items := make([]userListItem, 0) - var after *githubv4.String for { var query struct { Node struct { @@ -698,7 +755,7 @@ func AddRepositoryToList(t translations.TranslationHelperFunc) inventory.ServerT Required: []string{"owner", "repo", "list_name"}, }, }, - scopes.RequireAll(scopes.User), + scopes.RequireAll(scopes.User, scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { @@ -758,7 +815,7 @@ func RemoveRepositoryFromList(t translations.TranslationHelperFunc) inventory.Se Required: []string{"owner", "repo", "list_name"}, }, }, - scopes.RequireAll(scopes.User), + scopes.RequireAll(scopes.User, scopes.Repo), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { owner, err := RequiredParam[string](args, "owner") if err != nil { diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index b8fd6c378d..be3e478c62 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -78,7 +78,7 @@ func TestListUserLists(t *testing.T) { }, mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( - userListPageQuery{}, + userListPageWithItemsQuery{}, map[string]any{"after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ @@ -89,51 +89,20 @@ func TestListUserLists(t *testing.T) { "name": githubv4.String("My list"), "description": githubv4.String("A list"), "isPrivate": githubv4.Boolean(false), + "items": map[string]any{ + "nodes": []any{ + map[string]any{"nameWithOwner": githubv4.String("owner/repo")}, + }, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, + }, }, }, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, "totalCount": githubv4.Int(1), }, }, }), ), - githubv4mock.NewQueryMatcher( - struct { - Node struct { - UserList struct { - Items struct { - Nodes []struct { - Repository struct { - NameWithOwner githubv4.String - } `graphql:"... on Repository"` - } - PageInfo struct { - HasNextPage bool - EndCursor string - } - } `graphql:"items(first: 100, after: $after)"` - } `graphql:"... on UserList"` - } `graphql:"node(id: $id)"` - }{}, - map[string]any{ - "id": githubv4.ID("list-1"), - "after": (*githubv4.String)(nil), - }, - githubv4mock.DataResponse(map[string]any{ - "node": map[string]any{ - "items": map[string]any{ - "nodes": []any{ - map[string]any{ - "nameWithOwner": githubv4.String("owner/repo"), - }, - }, - "pageInfo": map[string]any{ - "hasNextPage": false, - "endCursor": "", - }, - }, - }, - }), - ), ), expectToolError: false, }, @@ -243,6 +212,31 @@ func TestListUserListsPaginates(t *testing.T) { require.Len(t, transport.calls, 2) } +func TestListUserListsIncludesFirstItemPageInlineAndContinues(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + assert.Nil(t, req.Variables["after"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-1","name":"List","description":"","isPrivate":false,"items":{"nodes":[{"nameWithOwner":"owner/first"}],"pageInfo":{"hasNextPage":true,"endCursor":"next-item"}}}],"pageInfo":{"hasNextPage":false,"endCursor":""},"totalCount":1}}}}` + }, + func(req capturedGraphQLRequest) (int, string) { + assert.Equal(t, githubv4.ID("list-1"), githubv4.ID(req.Variables["id"].(string))) + assert.Equal(t, "next-item", req.Variables["after"]) + return http.StatusOK, `{"data":{"node":{"items":{"nodes":[{"nameWithOwner":"owner/second"}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` + }, + }, + } + client := githubv4.NewClient(&http.Client{Transport: transport}) + + lists, totalCount, err := listUserLists(context.Background(), client, true) + require.NoError(t, err) + require.Len(t, lists, 1) + assert.Equal(t, 1, totalCount) + assert.Equal(t, []userListItem{{Repository: "owner/first"}, {Repository: "owner/second"}}, lists[0].Items) + require.Len(t, transport.calls, 2) +} + func TestCreateUserList(t *testing.T) { t.Parallel() @@ -569,7 +563,7 @@ func TestAddRepositoryToList(t *testing.T) { require.NotNil(t, tool.Annotations.DestructiveHint) assert.False(t, *tool.Annotations.DestructiveHint) assert.True(t, tool.Annotations.IdempotentHint) - assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + assert.Equal(t, []string{"user", "repo"}, serverTool.ScopeAccess.Scopes) // Repository currently in lists "A" and "B"; adding to "C" must resubmit all // three because updateUserListsForItem REPLACES membership (does not append). @@ -728,7 +722,7 @@ func TestRemoveRepositoryFromList(t *testing.T) { require.NotNil(t, tool.Annotations.DestructiveHint) assert.True(t, *tool.Annotations.DestructiveHint) assert.True(t, tool.Annotations.IdempotentHint) - assert.Equal(t, []string{"user"}, serverTool.ScopeAccess.Scopes) + assert.Equal(t, []string{"user", "repo"}, serverTool.ScopeAccess.Scopes) // Repository currently in lists "A" and "B"; removing from "B" must resubmit // only the remainder ("A"). From 5d355062572cf96ff00caefdb6f123562f1f5f3e Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:52:52 -0700 Subject: [PATCH 10/12] Preserve empty fetched user-list items Always serialize the items field so callers can distinguish metadata-only results (null) from an included list that was fetched and found empty ([]). Add focused JSON serialization coverage. --- pkg/github/user_lists.go | 2 +- pkg/github/user_lists_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 938333c256..05036dcea6 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -22,7 +22,7 @@ type userList struct { Name string `json:"name"` Description string `json:"description,omitempty"` IsPrivate bool `json:"is_private"` - Items []userListItem `json:"items,omitempty"` + Items []userListItem `json:"items"` } type userListItem struct { diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index be3e478c62..166bcf4cc7 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "encoding/json" "net/http" "testing" @@ -130,6 +131,18 @@ func TestListUserLists(t *testing.T) { } } +func TestUserListItemsJSONDistinguishesNotFetchedFromEmpty(t *testing.T) { + t.Parallel() + + notFetched, err := json.Marshal(userList{}) + require.NoError(t, err) + assert.Contains(t, string(notFetched), `"items":null`) + + fetchedEmpty, err := json.Marshal(userList{Items: []userListItem{}}) + require.NoError(t, err) + assert.Contains(t, string(fetchedEmpty), `"items":[]`) +} + func TestListUserListsIFCLabel(t *testing.T) { t.Parallel() From 209c966ea0c7543166ddff7e073d0c43c6e8a91a Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Wed, 2 Sep 2026 01:06:48 -0700 Subject: [PATCH 11/12] Bound user-list item-inclusive responses Expose standard cursor pagination on list_user_lists and return one bounded page of lists per call. When items are included, return at most the first 100 repositories per list together with itemsPageInfo instead of eagerly draining every nested connection. Update focused tests, tool snapshot, and generated documentation. --- README.md | 2 + pkg/github/__toolsnaps__/list_user_lists.snap | 10 + pkg/github/user_lists.go | 193 +++++++----------- pkg/github/user_lists_test.go | 44 ++-- 4 files changed, 105 insertions(+), 144 deletions(-) diff --git a/README.md b/README.md index c397d2fa38..df62325427 100644 --- a/README.md +++ b/README.md @@ -1507,7 +1507,9 @@ The following sets of tools are available: - **list_user_lists** - List star lists - **OAuth Challenge Scopes**: `read:user`, `repo` + - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `include_items`: Whether to include the repositories in each list. (boolean, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - **remove_repository_from_list** - Remove repository from star list - **OAuth Challenge Scopes**: `user`, `repo` diff --git a/pkg/github/__toolsnaps__/list_user_lists.snap b/pkg/github/__toolsnaps__/list_user_lists.snap index 8ca3ca213c..fa6de9bd61 100644 --- a/pkg/github/__toolsnaps__/list_user_lists.snap +++ b/pkg/github/__toolsnaps__/list_user_lists.snap @@ -7,9 +7,19 @@ "description": "List the authenticated user's star lists (UserLists), optionally including the repositories in each list.", "inputSchema": { "properties": { + "after": { + "description": "Cursor for pagination. Use the cursor from the previous response.", + "type": "string" + }, "include_items": { "description": "Whether to include the repositories in each list.", "type": "boolean" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" } }, "type": "object" diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index 05036dcea6..fa39a3f521 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -18,17 +18,25 @@ import ( // userList represents a GitHub star list (UserList) surfaced through the tools. type userList struct { - ID githubv4.ID `json:"id"` - Name string `json:"name"` - Description string `json:"description,omitempty"` - IsPrivate bool `json:"is_private"` - Items []userListItem `json:"items"` + ID githubv4.ID `json:"id"` + Name string `json:"name"` + Description string `json:"description,omitempty"` + IsPrivate bool `json:"is_private"` + Items []userListItem `json:"items"` + ItemsPageInfo *userListPageInfo `json:"itemsPageInfo,omitempty"` } type userListItem struct { Repository string `json:"repository"` } +type userListPageInfo struct { + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + StartCursor string `json:"startCursor,omitempty"` + EndCursor string `json:"endCursor,omitempty"` +} + type userListLookupQuery struct { Viewer struct { Lists struct { @@ -53,12 +61,9 @@ type userListPageQuery struct { Description githubv4.String IsPrivate githubv4.Boolean } - PageInfo struct { - HasNextPage bool - EndCursor string - } + PageInfo userListPageInfo TotalCount githubv4.Int - } `graphql:"lists(first: 100, after: $after)"` + } `graphql:"lists(first: $first, after: $after)"` } } @@ -76,18 +81,12 @@ type userListPageWithItemsQuery struct { NameWithOwner githubv4.String } `graphql:"... on Repository"` } - PageInfo struct { - HasNextPage bool - EndCursor string - } + PageInfo userListPageInfo } `graphql:"items(first: 100)"` } - PageInfo struct { - HasNextPage bool - EndCursor string - } + PageInfo userListPageInfo TotalCount githubv4.Int - } `graphql:"lists(first: 100, after: $after)"` + } `graphql:"lists(first: $first, after: $after)"` } } @@ -115,112 +114,49 @@ func getUserListID(ctx context.Context, client *githubv4.Client, name string) (g return "", fmt.Errorf("list '%s' not found", name) } -// listUserLists returns the authenticated user's star lists, optionally -// including the repositories each list contains. -func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bool) ([]userList, int, error) { - lists := make([]userList, 0) - totalCount := 0 - var after *githubv4.String - for { - if includeItems { - var query userListPageWithItemsQuery - vars := map[string]any{"after": after} - if err := client.Query(ctx, &query, vars); err != nil { - return nil, 0, err - } - totalCount = int(query.Viewer.Lists.TotalCount) - for _, node := range query.Viewer.Lists.Nodes { - items := make([]userListItem, 0, len(node.Items.Nodes)) - for _, item := range node.Items.Nodes { - items = append(items, userListItem{Repository: string(item.Repository.NameWithOwner)}) - } - if node.Items.PageInfo.HasNextPage { - cursor := githubv4.String(node.Items.PageInfo.EndCursor) - remaining, err := listUserListItems(ctx, client, node.ID, &cursor) - if err != nil { - return nil, 0, err - } - items = append(items, remaining...) - } - lists = append(lists, userList{ - ID: node.ID, - Name: string(node.Name), - Description: string(node.Description), - IsPrivate: bool(node.IsPrivate), - Items: items, - }) - } - if !query.Viewer.Lists.PageInfo.HasNextPage { - break - } - cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) - after = &cursor - continue - } - - var query userListPageQuery - vars := map[string]any{"after": after} +// listUserLists returns one bounded page of the authenticated user's star +// lists. When includeItems is true, each list includes at most its first 100 +// repositories plus cursor metadata indicating whether more items exist. +func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bool, first githubv4.Int, after *githubv4.String) ([]userList, int, userListPageInfo, error) { + vars := map[string]any{"first": first, "after": after} + if includeItems { + var query userListPageWithItemsQuery if err := client.Query(ctx, &query, vars); err != nil { - return nil, 0, err + return nil, 0, userListPageInfo{}, err } - totalCount = int(query.Viewer.Lists.TotalCount) + lists := make([]userList, 0, len(query.Viewer.Lists.Nodes)) for _, node := range query.Viewer.Lists.Nodes { - list := userList{ - ID: node.ID, - Name: string(node.Name), - Description: string(node.Description), - IsPrivate: bool(node.IsPrivate), - } - lists = append(lists, list) - } - if !query.Viewer.Lists.PageInfo.HasNextPage { - break + items := make([]userListItem, 0, len(node.Items.Nodes)) + for _, item := range node.Items.Nodes { + items = append(items, userListItem{Repository: string(item.Repository.NameWithOwner)}) + } + itemsPageInfo := node.Items.PageInfo + lists = append(lists, userList{ + ID: node.ID, + Name: string(node.Name), + Description: string(node.Description), + IsPrivate: bool(node.IsPrivate), + Items: items, + ItemsPageInfo: &itemsPageInfo, + }) } - cursor := githubv4.String(query.Viewer.Lists.PageInfo.EndCursor) - after = &cursor + return lists, int(query.Viewer.Lists.TotalCount), query.Viewer.Lists.PageInfo, nil } - return lists, totalCount, nil -} -// listUserListItems returns the repositories held by a single list, following -// the items connection's cursor until every page has been consumed. -func listUserListItems(ctx context.Context, client *githubv4.Client, listID githubv4.ID, after *githubv4.String) ([]userListItem, error) { - items := make([]userListItem, 0) - for { - var query struct { - Node struct { - UserList struct { - Items struct { - Nodes []struct { - Repository struct { - NameWithOwner githubv4.String - } `graphql:"... on Repository"` - } - PageInfo struct { - HasNextPage bool - EndCursor string - } - } `graphql:"items(first: 100, after: $after)"` - } `graphql:"... on UserList"` - } `graphql:"node(id: $id)"` - } - vars := map[string]any{ - "id": listID, - "after": after, - } - if err := client.Query(ctx, &query, vars); err != nil { - return nil, err - } - for _, node := range query.Node.UserList.Items.Nodes { - items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)}) - } - if !query.Node.UserList.Items.PageInfo.HasNextPage { - break - } - cursor := githubv4.String(query.Node.UserList.Items.PageInfo.EndCursor) - after = &cursor + var query userListPageQuery + if err := client.Query(ctx, &query, vars); err != nil { + return nil, 0, userListPageInfo{}, err } - return items, nil + lists := make([]userList, 0, len(query.Viewer.Lists.Nodes)) + for _, node := range query.Viewer.Lists.Nodes { + lists = append(lists, userList{ + ID: node.ID, + Name: string(node.Name), + Description: string(node.Description), + IsPrivate: bool(node.IsPrivate), + }) + } + return lists, int(query.Viewer.Lists.TotalCount), query.Viewer.Lists.PageInfo, nil } // repoInList reports whether the repository identified by repoID belongs to the @@ -494,7 +430,7 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { Title: t("TOOL_LIST_USER_LISTS_USER_TITLE", "List star lists"), ReadOnlyHint: true, }, - InputSchema: &jsonschema.Schema{ + InputSchema: WithCursorPagination(&jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ "include_items": { @@ -502,7 +438,7 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { Description: "Whether to include the repositories in each list.", }, }, - }, + }), }, userListReadScopeAccess(), func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { @@ -510,19 +446,34 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + pagination, err := OptionalCursorPaginationParams(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + paginationParams, err := pagination.ToGraphQLParams() + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + first := githubv4.Int(*paginationParams.First) + var after *githubv4.String + if paginationParams.After != nil { + cursor := githubv4.String(*paginationParams.After) + after = &cursor + } client, err := deps.GetGQLClient(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) } - lists, totalCount, err := listUserLists(ctx, client, includeItems) + lists, totalCount, pageInfo, err := listUserLists(ctx, client, includeItems, first, after) if err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to list user lists", err), nil, nil } response := map[string]any{ "lists": lists, + "pageInfo": pageInfo, "totalCount": totalCount, } out, err := json.Marshal(response) diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index 166bcf4cc7..e175f06dc4 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -52,7 +52,7 @@ func TestListUserLists(t *testing.T) { mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( userListPageQuery{}, - map[string]any{"after": (*githubv4.String)(nil)}, + map[string]any{"first": githubv4.Int(30), "after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -80,7 +80,7 @@ func TestListUserLists(t *testing.T) { mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( userListPageWithItemsQuery{}, - map[string]any{"after": (*githubv4.String)(nil)}, + map[string]any{"first": githubv4.Int(30), "after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -150,7 +150,7 @@ func TestListUserListsIFCLabel(t *testing.T) { mockedClient := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( userListPageQuery{}, - map[string]any{"after": (*githubv4.String)(nil)}, + map[string]any{"first": githubv4.Int(30), "after": (*githubv4.String)(nil)}, githubv4mock.DataResponse(map[string]any{ "viewer": map[string]any{ "lists": map[string]any{ @@ -200,54 +200,52 @@ func TestGetUserListIDPaginates(t *testing.T) { require.Len(t, transport.calls, 2) } -func TestListUserListsPaginates(t *testing.T) { +func TestListUserListsReturnsRequestedPage(t *testing.T) { transport := &sequencedGraphQLTransport{ t: t, responses: []func(capturedGraphQLRequest) (int, string){ - func(req capturedGraphQLRequest) (int, string) { - assert.Nil(t, req.Variables["after"]) - return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-1","name":"First","description":"","isPrivate":false}],"pageInfo":{"hasNextPage":true,"endCursor":"next-list"},"totalCount":2}}}}` - }, func(req capturedGraphQLRequest) (int, string) { assert.Equal(t, "next-list", req.Variables["after"]) - return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-2","name":"Second","description":"","isPrivate":true}],"pageInfo":{"hasNextPage":false,"endCursor":""},"totalCount":2}}}}` + assert.Equal(t, float64(1), req.Variables["first"]) + return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-2","name":"Second","description":"","isPrivate":true}],"pageInfo":{"hasNextPage":false,"hasPreviousPage":true,"startCursor":"second","endCursor":"second"},"totalCount":2}}}}` }, }, } client := githubv4.NewClient(&http.Client{Transport: transport}) + after := githubv4.String("next-list") - lists, totalCount, err := listUserLists(context.Background(), client, false) + lists, totalCount, pageInfo, err := listUserLists(context.Background(), client, false, 1, &after) require.NoError(t, err) - require.Len(t, lists, 2) - assert.Equal(t, "First", lists[0].Name) - assert.Equal(t, "Second", lists[1].Name) + require.Len(t, lists, 1) + assert.Equal(t, "Second", lists[0].Name) assert.Equal(t, 2, totalCount) - require.Len(t, transport.calls, 2) + assert.True(t, pageInfo.HasPreviousPage) + assert.Equal(t, "second", pageInfo.EndCursor) + require.Len(t, transport.calls, 1) } -func TestListUserListsIncludesFirstItemPageInlineAndContinues(t *testing.T) { +func TestListUserListsIncludesBoundedItemPage(t *testing.T) { transport := &sequencedGraphQLTransport{ t: t, responses: []func(capturedGraphQLRequest) (int, string){ func(req capturedGraphQLRequest) (int, string) { assert.Nil(t, req.Variables["after"]) + assert.Equal(t, float64(30), req.Variables["first"]) return http.StatusOK, `{"data":{"viewer":{"lists":{"nodes":[{"id":"list-1","name":"List","description":"","isPrivate":false,"items":{"nodes":[{"nameWithOwner":"owner/first"}],"pageInfo":{"hasNextPage":true,"endCursor":"next-item"}}}],"pageInfo":{"hasNextPage":false,"endCursor":""},"totalCount":1}}}}` }, - func(req capturedGraphQLRequest) (int, string) { - assert.Equal(t, githubv4.ID("list-1"), githubv4.ID(req.Variables["id"].(string))) - assert.Equal(t, "next-item", req.Variables["after"]) - return http.StatusOK, `{"data":{"node":{"items":{"nodes":[{"nameWithOwner":"owner/second"}],"pageInfo":{"hasNextPage":false,"endCursor":""}}}}}` - }, }, } client := githubv4.NewClient(&http.Client{Transport: transport}) - lists, totalCount, err := listUserLists(context.Background(), client, true) + lists, totalCount, _, err := listUserLists(context.Background(), client, true, 30, nil) require.NoError(t, err) require.Len(t, lists, 1) assert.Equal(t, 1, totalCount) - assert.Equal(t, []userListItem{{Repository: "owner/first"}, {Repository: "owner/second"}}, lists[0].Items) - require.Len(t, transport.calls, 2) + assert.Equal(t, []userListItem{{Repository: "owner/first"}}, lists[0].Items) + require.NotNil(t, lists[0].ItemsPageInfo) + assert.True(t, lists[0].ItemsPageInfo.HasNextPage) + assert.Equal(t, "next-item", lists[0].ItemsPageInfo.EndCursor) + require.Len(t, transport.calls, 1) } func TestCreateUserList(t *testing.T) { From d0bb8a1b74ba69159374e76a1d96e410f7a12c3d Mon Sep 17 00:00:00 2001 From: ppoffice <8849362+ppoffice@users.noreply.github.com> Date: Wed, 2 Sep 2026 11:29:11 -0700 Subject: [PATCH 12/12] Add paginated user-list item retrieval Require read:user visibility for classic PATs, add a dedicated cursor-paginated list_user_list_items tool for continuing item pages, reject missing repository IDs before membership scans, and assert the user scope remains opt-in. Update tests, snapshots, and generated documentation. --- README.md | 8 +- .../__toolsnaps__/list_user_list_items.snap | 31 +++++ pkg/github/__toolsnaps__/list_user_lists.snap | 4 +- pkg/github/tool_scopes.go | 6 +- pkg/github/tools.go | 1 + pkg/github/user_lists.go | 109 +++++++++++++++++- pkg/github/user_lists_test.go | 99 +++++++++++++++- pkg/scopes/scopes_test.go | 2 + 8 files changed, 249 insertions(+), 11 deletions(-) create mode 100644 pkg/github/__toolsnaps__/list_user_list_items.snap diff --git a/README.md b/README.md index df62325427..a7ffdbc6c7 100644 --- a/README.md +++ b/README.md @@ -1505,10 +1505,16 @@ The following sets of tools are available: - `sort`: How to sort the results. Can be either 'created' (when the repository was starred) or 'updated' (when the repository was last pushed to). (string, optional) - `username`: Username to list starred repositories for. Defaults to the authenticated user. (string, optional) +- **list_user_list_items** - List star list items + - **OAuth Challenge Scopes**: `read:user`, `repo` + - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) + - `name`: The name of the star list whose repositories should be listed. (string, required) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - **list_user_lists** - List star lists - **OAuth Challenge Scopes**: `read:user`, `repo` - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - - `include_items`: Whether to include the repositories in each list. (boolean, optional) + - `include_items`: Whether to include up to 100 repositories and item cursor metadata for each returned list. (boolean, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - **remove_repository_from_list** - Remove repository from star list diff --git a/pkg/github/__toolsnaps__/list_user_list_items.snap b/pkg/github/__toolsnaps__/list_user_list_items.snap new file mode 100644 index 0000000000..69d12ade25 --- /dev/null +++ b/pkg/github/__toolsnaps__/list_user_list_items.snap @@ -0,0 +1,31 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "List star list items" + }, + "description": "List a page of repositories in one star list (UserList).", + "inputSchema": { + "properties": { + "after": { + "description": "Cursor for pagination. Use the cursor from the previous response.", + "type": "string" + }, + "name": { + "description": "The name of the star list whose repositories should be listed.", + "type": "string" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + } + }, + "required": [ + "name" + ], + "type": "object" + }, + "name": "list_user_list_items" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_user_lists.snap b/pkg/github/__toolsnaps__/list_user_lists.snap index fa6de9bd61..34fd17eef7 100644 --- a/pkg/github/__toolsnaps__/list_user_lists.snap +++ b/pkg/github/__toolsnaps__/list_user_lists.snap @@ -4,7 +4,7 @@ "readOnlyHint": true, "title": "List star lists" }, - "description": "List the authenticated user's star lists (UserLists), optionally including the repositories in each list.", + "description": "List a page of the authenticated user's star lists (UserLists), optionally including the first page of repositories in each list.", "inputSchema": { "properties": { "after": { @@ -12,7 +12,7 @@ "type": "string" }, "include_items": { - "description": "Whether to include the repositories in each list.", + "description": "Whether to include up to 100 repositories and item cursor metadata for each returned list.", "type": "boolean" }, "perPage": { diff --git a/pkg/github/tool_scopes.go b/pkg/github/tool_scopes.go index 480ddd947a..0790fdab3e 100644 --- a/pkg/github/tool_scopes.go +++ b/pkg/github/tool_scopes.go @@ -73,10 +73,8 @@ func uiGetScopeAccess() inventory.ScopeAccess { func userListReadScopeAccess() inventory.ScopeAccess { return scopes.DynamicChallenge( []scopes.Scope{scopes.ReadUser, scopes.Repo}, - func([]string) bool { - // User-list metadata may be readable with read:user, while MCP OAuth - // can challenge for missing scopes at call time. - return true + func(activeScopes []string) bool { + return scopes.HasAll(activeScopes, scopes.ReadUser) }, func(arguments map[string]any, activeScopes []string) []string { includeItems, ok := arguments["include_items"].(bool) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 78b9256897..c2f8798b84 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -242,6 +242,7 @@ func AllTools(t translations.TranslationHelperFunc, opts ...ToolOption) []invent StarRepository(t), UnstarRepository(t), ListUserLists(t), + ListUserListItems(t), CreateUserList(t), UpdateUserList(t), DeleteUserList(t), diff --git a/pkg/github/user_lists.go b/pkg/github/user_lists.go index fa39a3f521..a9c1fe5b8f 100644 --- a/pkg/github/user_lists.go +++ b/pkg/github/user_lists.go @@ -90,6 +90,22 @@ type userListPageWithItemsQuery struct { } } +type userListItemsPageQuery struct { + Node struct { + UserList struct { + Items struct { + Nodes []struct { + Repository struct { + NameWithOwner githubv4.String + } `graphql:"... on Repository"` + } + PageInfo userListPageInfo + TotalCount githubv4.Int + } `graphql:"items(first: $first, after: $after)"` + } `graphql:"... on UserList"` + } `graphql:"node(id: $id)"` +} + // getUserListID resolves the authenticated user's list with the given name to // its node ID. It returns an error when no list matches the name. func getUserListID(ctx context.Context, client *githubv4.Client, name string) (githubv4.ID, error) { @@ -159,6 +175,23 @@ func listUserLists(ctx context.Context, client *githubv4.Client, includeItems bo return lists, int(query.Viewer.Lists.TotalCount), query.Viewer.Lists.PageInfo, nil } +func listUserListItemsPage(ctx context.Context, client *githubv4.Client, listID githubv4.ID, first githubv4.Int, after *githubv4.String) ([]userListItem, int, userListPageInfo, error) { + var query userListItemsPageQuery + var afterVariable any = (*githubv4.String)(nil) + if after != nil { + afterVariable = *after + } + vars := map[string]any{"id": listID, "first": first, "after": afterVariable} + if err := client.Query(ctx, &query, vars); err != nil { + return nil, 0, userListPageInfo{}, err + } + items := make([]userListItem, 0, len(query.Node.UserList.Items.Nodes)) + for _, node := range query.Node.UserList.Items.Nodes { + items = append(items, userListItem{Repository: string(node.Repository.NameWithOwner)}) + } + return items, int(query.Node.UserList.Items.TotalCount), query.Node.UserList.Items.PageInfo, nil +} + // repoInList reports whether the repository identified by repoID belongs to the // list identified by listID, paging through the list's items until a match is // found or the connection is exhausted. @@ -311,6 +344,9 @@ func setRepoListMemberships(ctx context.Context, client *githubv4.Client, owner, if err != nil { return fmt.Errorf("failed to find repository: %w", err) } + if repoID == nil || repoID == "" { + return fmt.Errorf("repository '%s/%s' not found", owner, repo) + } // Walk every list and, for each, every page of items to determine which // lists currently contain the repository. @@ -425,7 +461,7 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { ToolsetMetadataStargazers, mcp.Tool{ Name: "list_user_lists", - Description: t("TOOL_LIST_USER_LISTS_DESCRIPTION", "List the authenticated user's star lists (UserLists), optionally including the repositories in each list."), + Description: t("TOOL_LIST_USER_LISTS_DESCRIPTION", "List a page of the authenticated user's star lists (UserLists), optionally including the first page of repositories in each list."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_LIST_USER_LISTS_USER_TITLE", "List star lists"), ReadOnlyHint: true, @@ -435,7 +471,7 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { Properties: map[string]*jsonschema.Schema{ "include_items": { Type: "boolean", - Description: "Whether to include the repositories in each list.", + Description: "Whether to include up to 100 repositories and item cursor metadata for each returned list.", }, }, }), @@ -487,6 +523,75 @@ func ListUserLists(t translations.TranslationHelperFunc) inventory.ServerTool { ) } +// ListUserListItems creates a tool to page through the repositories in one +// user list. Use the per-list cursor returned by list_user_lists when its item +// preview indicates that additional pages exist. +func ListUserListItems(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataStargazers, + mcp.Tool{ + Name: "list_user_list_items", + Description: t("TOOL_LIST_USER_LIST_ITEMS_DESCRIPTION", "List a page of repositories in one star list (UserList)."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_USER_LIST_ITEMS_USER_TITLE", "List star list items"), + ReadOnlyHint: true, + }, + InputSchema: WithCursorPagination(&jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The name of the star list whose repositories should be listed.", + }, + }, + Required: []string{"name"}, + }), + }, + scopes.RequireAll(scopes.ReadUser, scopes.Repo), + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + name, err := RequiredParam[string](args, "name") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + pagination, err := OptionalCursorPaginationParams(args) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + paginationParams, err := pagination.ToGraphQLParams() + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + first := githubv4.Int(*paginationParams.First) + var after *githubv4.String + if paginationParams.After != nil { + cursor := githubv4.String(*paginationParams.After) + after = &cursor + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + listID, err := getUserListID(ctx, client, name) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find user list", err), nil, nil + } + items, totalCount, pageInfo, err := listUserListItemsPage(ctx, client, listID, first, after) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to list user-list items", err), nil, nil + } + response := map[string]any{ + "items": items, + "pageInfo": pageInfo, + "totalCount": totalCount, + } + result := MarshalledTextResult(response) + result = attachStaticIFCLabel(ctx, deps, result, ifc.LabelUserList()) + return result, nil, nil + }, + ) +} + // CreateUserList creates a tool to create a new star list. func CreateUserList(t translations.TranslationHelperFunc) inventory.ServerTool { return NewTool( diff --git a/pkg/github/user_lists_test.go b/pkg/github/user_lists_test.go index e175f06dc4..6be676037c 100644 --- a/pkg/github/user_lists_test.go +++ b/pkg/github/user_lists_test.go @@ -32,7 +32,8 @@ func TestListUserLists(t *testing.T) { assert.NotNil(t, serverTool.ScopeAccess.Visible) assert.NotNil(t, serverTool.ScopeAccess.Challenge) assert.True(t, serverTool.ScopeAccess.Dynamic) - assert.True(t, serverTool.ScopeAccess.Visible(nil)) + assert.False(t, serverTool.ScopeAccess.Visible(nil)) + assert.True(t, serverTool.ScopeAccess.Visible([]string{"read:user"})) assert.Empty(t, serverTool.ScopeAccess.Challenge(map[string]any{}, []string{"read:user"})) assert.ElementsMatch(t, []string{"read:user", "repo"}, serverTool.ScopeAccess.Challenge( map[string]any{"include_items": true}, []string{"read:user"}, @@ -171,7 +172,7 @@ func TestListUserListsIFCLabel(t *testing.T) { result, err := handler(ContextWithDeps(context.Background(), deps), &request) require.NoError(t, err) - require.False(t, result.IsError) + require.False(t, result.IsError, "unexpected tool error: %s", getTextResult(t, result).Text) label, ok := result.Meta["ifc"].(ifc.SecurityLabel) require.True(t, ok) @@ -248,6 +249,62 @@ func TestListUserListsIncludesBoundedItemPage(t *testing.T) { require.Len(t, transport.calls, 1) } +func TestListUserListItems(t *testing.T) { + t.Parallel() + + serverTool := ListUserListItems(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + assert.Equal(t, "list_user_list_items", tool.Name) + assert.True(t, tool.Annotations.ReadOnlyHint) + assert.Equal(t, []string{"read:user", "repo"}, serverTool.ScopeAccess.Scopes) + + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{map[string]any{"id": githubv4.ID("list-1"), "name": githubv4.String("List")}}, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + userListItemsPageQuery{}, + map[string]any{ + "id": githubv4.ID("list-1"), + "first": githubv4.Int(1), + "after": githubv4.String("item-start"), + }, + githubv4mock.DataResponse(map[string]any{ + "node": map[string]any{ + "items": map[string]any{ + "nodes": []any{map[string]any{"nameWithOwner": githubv4.String("owner/repo")}}, + "pageInfo": map[string]any{ + "hasNextPage": false, + "endCursor": "item-cursor", + }, + "totalCount": githubv4.Int(1), + }, + }, + }), + ), + ) + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{"name": "List", "perPage": float64(1), "after": "item-start"}) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, "unexpected tool error: %s", getTextResult(t, result).Text) + assert.Contains(t, getTextResult(t, result).Text, `"repository":"owner/repo"`) + assert.Contains(t, getTextResult(t, result).Text, `"endCursor":"item-cursor"`) +} + func TestCreateUserList(t *testing.T) { t.Parallel() @@ -901,6 +958,44 @@ func TestAddRepositoryToListListNotFound(t *testing.T) { assert.Contains(t, textContent.Text, "list 'Missing' not found") } +func TestAddRepositoryToListRepositoryNotFound(t *testing.T) { + t.Parallel() + + serverTool := AddRepositoryToList(translations.NullTranslationHelper) + mockedClient := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + userListLookupQuery{}, + map[string]any{"after": (*githubv4.String)(nil)}, + githubv4mock.DataResponse(map[string]any{ + "viewer": map[string]any{ + "lists": map[string]any{ + "nodes": []any{map[string]any{"id": githubv4.ID("list-c"), "name": githubv4.String("C")}}, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + ID githubv4.ID + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{"owner": githubv4.String("owner"), "repo": githubv4.String("repo")}, + githubv4mock.DataResponse(map[string]any{"repository": nil}), + ), + ) + client := githubv4.NewClient(mockedClient) + deps := BaseDeps{GQLClient: client} + handler := serverTool.Handler(deps) + request := createMCPRequest(map[string]any{"owner": "owner", "repo": "repo", "list_name": "C"}) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, getErrorResult(t, result).Text, "repository 'owner/repo' not found") +} + func TestSetRepoListMembershipsSkipsSatisfiedMutation(t *testing.T) { tests := []struct { name string diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index 62bfc6178e..2af00e3e98 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -17,6 +17,8 @@ func TestOAuthScopeCatalog(t *testing.T) { assert.NotContains(t, defaults, string(Workflow)) assert.Contains(t, supported, string(Codespace)) assert.NotContains(t, defaults, string(Codespace)) + assert.Contains(t, supported, string(User)) + assert.NotContains(t, defaults, string(User)) } func TestScopeChecks(t *testing.T) {