Add star list (UserList) management tools - #3200
Closed
ppoffice wants to merge 12 commits into
Closed
Conversation
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).
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.
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unpaginated membership discovery can silently remove existing list memberships, alongside several smaller API and cleanup issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds GitHub star-list management to the existing stargazers toolset.
Changes:
- Adds six GraphQL-backed list management tools.
- Introduces opt-in
userOAuth scope support. - Adds unit/E2E coverage, snapshots, and generated documentation.
File summaries
| File | Description |
|---|---|
README.md |
Documents the new tools. |
pkg/scopes/scopes.go |
Registers the user scope. |
pkg/http/oauth/oauth_test.go |
Updates supported-scope expectations. |
pkg/github/user_lists.go |
Implements list operations and tools. |
pkg/github/user_lists_test.go |
Tests schemas and behavior. |
pkg/github/tools.go |
Registers the tools globally. |
pkg/github/__toolsnaps__/update_user_list.snap |
Snapshots update schema. |
pkg/github/__toolsnaps__/remove_repository_from_list.snap |
Snapshots removal schema. |
pkg/github/__toolsnaps__/list_user_lists.snap |
Snapshots listing schema. |
pkg/github/__toolsnaps__/delete_user_list.snap |
Snapshots deletion schema. |
pkg/github/__toolsnaps__/create_user_list.snap |
Snapshots creation schema. |
pkg/github/__toolsnaps__/add_repository_to_list.snap |
Snapshots addition schema. |
e2e/e2e_test.go |
Tests the complete list lifecycle. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Replacement-style membership updates can lose concurrent changes, and critical pagination behavior lacks coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/scopes/scopes.go:90
- Please assert that
Useris absent fromDefaultOAuthScopes, as is done for the other opt-in scopes inpkg/scopes/scopes_test.go:14-19. The current supported-scope test would not catch an accidental future change that requests this write-capable scope by default.
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Private list results need IFC confidentiality labeling, and large-list membership scans unnecessarily repeat requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
List pagination is incomplete and OAuth scope declarations do not cover private repository access.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/github/user_lists.go:670
- The handler resolves the repository through GraphQL, so adding a private repository requires
repoaccess in addition to theuserscope needed for the list mutation. With only the advertiseduserscope, the tool is exposed but fails for private repositories. Require both scopes, consistent with other repository mutation tools.
This issue also appears on line 730 of the same file.
pkg/github/user_lists.go:69
list_user_listssilently truncates the result to the first 100 lists while returning the fulltotalCount, and the tool exposes no cursor with which callers can fetch the remainder. Paginate this connection internally (as the membership path already does) or expose pagination parameters and cursor metadata.
} `graphql:"lists(first: 100)"`
pkg/github/user_lists.go:730
- Removing a private repository also performs a repository lookup that requires
repo, but this declaration advertises and checks onlyuser. Tokens satisfying the declared scope can therefore invoke the tool and fail on private repositories. Require bothuserandrepohere.
scopes.RequireAll(scopes.User),
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Membership writes can omit private memberships without repo scope and can overwrite concurrent changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
pkg/github/user_lists.go:761
- This remove path has the same missing
reporequirement as the add path: it scans all repository memberships and then replaces the complete set. A token with onlyusercan omit private items (and may not resolve the target private repository), causing a false no-op or loss of unseen memberships. Require bothuserandrepo.
scopes.RequireAll(scopes.User),
pkg/github/user_lists.go:417
- The read-modify-write is not atomic: another client can change this repository's list memberships after the scan, and this replacement mutation will silently overwrite that unrelated change. Since the tool promises a single-list add/remove, use an atomic API if one is available, or introduce a conflict-detection strategy that fails rather than submitting a stale complete set.
input := githubv4.UpdateUserListsForItemInput{
ItemID: repoID,
ListIDs: result,
}
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Empty included lists currently omit the items field, making fetched-empty and not-fetched results indistinguishable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Item-inclusive listing has unbounded API and response growth because it fully drains every list without pagination controls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Missing-repository handling, PAT scope visibility, unpageable items, and lost-update races affect correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/github/user_lists.go:313
getRepositoryIDreturns the zero ID without an error when GraphQL resolves a missing repository tonull. In that case add proceeds to an invalid mutation, while remove seespresent == falseand incorrectly reports success. Reject an empty repository ID before scanning memberships.
pkg/github/user_lists.go:410
- The replacement mutation is based on a membership snapshot read several requests earlier. If another MCP call or GitHub client changes this repository's list memberships before this mutation, the stale
ListIDsvalue silently erases that concurrent change. The add/remove contract needs a concurrency strategy (and, if GitHub offers no conditional mutation, the unavoidable external race should be reflected in the API rather than presented as a safe atomic add/remove).
input := githubv4.UpdateUserListsForItemInput{
ItemID: repoID,
ListIDs: result,
}
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
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.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Membership replacement can lose concurrent updates, and ambiguous rename failures can leak test lists.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
e2e/e2e_test.go:2028
- Cleanup can leak the globally scoped list when the rename commits on GitHub but
CallToolfails before returning:currentListNameremains the old name, so cleanup never tries the renamed name. Attempt cleanup with both the tracked name andrenamedListwhen rename completion is uncertain.
pkg/github/user_lists.go:445
- This read-modify-write is not atomic. Two concurrent add/remove calls for the same repository can both read the same memberships and then replace them with different stale snapshots, so whichever mutation runs last silently removes the other call's update. Please serialize membership updates per repository (including across server instances, if applicable), or otherwise change the contract/implementation so concurrent updates cannot be reported as successfully preserved.
input := githubv4.UpdateUserListsForItemInput{
ItemID: repoID,
ListIDs: result,
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds support for managing GitHub star lists (the UserList feature at github.com/stars) to the MCP server, folding 6 new tools into the existing
stargazerstoolset.list_user_listsinclude_items?viewer { lists }(+node(id){...on UserList{ items }})read:usercreate_user_listname,description?,is_private?createUserListuserupdate_user_listname,new_name?,description?,is_private?updateUserListuserdelete_user_listnamedeleteUserListuseradd_repository_to_listowner,repo,list_nameupdateUserListsForItemuserremove_repository_from_listowner,repo,list_nameupdateUserListsForItemuserDesign notes
updateUserListsForItemreplaces a repository's full list membership (does not append). The add/remove tools therefore do a read-modify-write: they walk the viewer's lists to derive the repository's current memberships, merge/subtract the target list, and resubmit the complete set.listsfield onRepository), which is why membership is derived from the list side viaviewer { lists { items } }.userOAuth scope is opt-in (not in the default scope set).Changes
pkg/github/user_lists.go— GraphQL helpers + 6 tool constructors.pkg/github/tools.go— register the 6 tools inAllTools.pkg/scopes/scopes.go— adduserscope (opt-in).pkg/github/user_lists_test.go— snapshot + behavioral unit tests (name→ID resolution, replace-vs-append merge logic, scope gating).e2e/e2e_test.go—TestUserListsexercising the full create → rename → add → verify → remove → delete lifecycle.pkg/http/oauth/oauth_test.go— updateTestSupportedScopes.Verification
go build ./...passes.go vet -tags e2e ./e2e/passes.TestUserListsverified against the live GitHub API (in-process viaGITHUB_MCP_SERVER_E2E_DEBUG=true).