-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add get_prs_reviewed_by tool for direct review lookup #1971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
3da1dd9 to
72b6dc8
Compare
| mcp.Tool{ | ||
| Name: "list_pull_requests", | ||
| Description: t("TOOL_LIST_PULL_REQUESTS_DESCRIPTION", "List pull requests in a GitHub repository. If the user specifies an author, then DO NOT use this tool and use the search_pull_requests tool instead."), | ||
| Description: t("TOOL_LIST_PULL_REQUESTS_DESCRIPTION", "List pull requests in a GitHub repository. If the user specifies an author, then DO NOT use this tool and use the search_pull_requests tool instead. If you receive a 422 error from search_pull_requests, then use the get_prs_reviewed_by tool (to list by reviewer), or this tool with the author parameter (for filtering by author) depending on what you need."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for TOOL_LIST_PULL_REQUESTS_DESCRIPTION is getting a bit long and complex. Consider rephrasing for clarity or splitting it into multiple sentences.
| } | ||
| } | ||
| prs = filtered | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments for the new function GetPRsReviewedBy to explain the high-level logic.
| assert.Contains(t, schema.Properties, "owner") | ||
| assert.Contains(t, schema.Properties, "repo") | ||
| assert.Contains(t, schema.Properties, "reviewer") | ||
| assert.Contains(t, schema.Properties, "state") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments to explain what the different mock reviews represent, this will help future maintainers.
Summary
This PR addresses a (seemingly bug?) I've encountered with the github API when searching for PR data for specific users, using two patches. I apologize if this is actually caused by some issue on my end, but it may also be a legitimate bug in the github search functionality. Many users seem to state that the search functionality is pretty broken, so I'm not discounting the possibility that this is a bug. Experience has shown that github doesn't necessarily address user requests in a reasonable amount of time, so I came up with some workarounds here.
I'll also say that due to the stochastic nature of LLMs, I really can't say that the hints/instructions in the tool defs will result in entirely reliable steering. I can say that they did in my extremely limited testing, but YMMV. I'm also entirely okay with keeping this as my own private fork if the community feels that this patch is unnecessary to the userbase.
Add get_prs_reviewed_by
Add new
get_prs_reviewed_bytool that finds PRs reviewed by a specific user using direct API calls instead of the Search API.This is a workaround for when
reviewed-by:qualifier doesn't work for some user accounts.Problem
The GitHub Search API
reviewed-by:qualifier returns 422 errors for some user accounts:While I can't find a direct reference to this (seemingly) bug elsewhere, I can confirm that the standard search endpoint fails to discover a user in my org, while listing pull requests and filtering does the trick.
Solution
client.PullRequests.List()client.PullRequests.ListReviews()Downsides are that the owner and repo must be specified. I've updated the tool help for some of the other related tools that fail in this situation so that the LLM agent using the MCP server will hopefully guide itself to trying this tool alternatively if it receives a 422 error. I apologize if this is not proper, but it did work well in limited testing for me (as can be seen in test plan).
Test Plan
Using claude code:
Modify list_pull_requests tool to add local/client side author filtering
Modify the
list_pull_requeststool to add client side author filtering.This is a workaround for when querying PRs for a specific user using the search endpoint fails.
Problem
The GitHub Search API
/issues?q=authorendpoint returns 422 errors for some user accounts:This involves the same search endpoint that returns the 422 error from part one of this PR.
Solution
Downsides are that filtering on the client side may introduce some minor amount of latency. I have not benchmarked this but it's likely minor.