Skip to content

adding review comments grouped as threads #1554

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.

Sign up for GitHub

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

Draft
tonytrg wants to merge 1 commit into
base: main
Choose a base branch
from
+358 −144
Draft

adding review comments grouped as threads #1554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ Possible options:
2. get_diff - Get the diff of a pull request.
3. get_status - Get status of a head commit in a pull request. This reflects status of builds and checks.
4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.
5. get_review_comments - Get the review comments on a pull request. They are comments made on a portion of the unified diff during a pull request review. Use with pagination parameters to control the number of results returned.
5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.
7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.
(string, required)
Expand Down
2 changes: 1 addition & 1 deletion pkg/github/__toolsnaps__/pull_request_read.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"properties": {
"method": {
"type": "string",
"description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get status of a head commit in a pull request. This reflects status of builds and checks.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_review_comments - Get the review comments on a pull request. They are comments made on a portion of the unified diff during a pull request review. Use with pagination parameters to control the number of results returned.\n 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.\n 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n",
"description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get status of a head commit in a pull request. This reflects status of builds and checks.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.\n 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.\n 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n",
"enum": [
"get",
"get_diff",
Expand Down
152 changes: 116 additions & 36 deletions pkg/github/pullrequests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
)

// PullRequestRead creates a tool to get details of a specific pull request.
func PullRequestRead(getClient GetClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
func PullRequestRead(getClient GetClientFn, getGQLClient GetGQLClientFn, cache *lockdown.RepoAccessCache, t translations.TranslationHelperFunc, flags FeatureFlags) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
schema := &jsonschema.Schema{
Type: "object",
Properties: map[string]*jsonschema.Schema{
Expand All @@ -33,7 +33,7 @@ Possible options:
2. get_diff - Get the diff of a pull request.
3. get_status - Get status of a head commit in a pull request. This reflects status of builds and checks.
4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.
5. get_review_comments - Get the review comments on a pull request. They are comments made on a portion of the unified diff during a pull request review. Use with pagination parameters to control the number of results returned.
5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.
6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method.
7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.
`,
Expand Down Expand Up @@ -107,7 +107,11 @@ Possible options:
result, err := GetPullRequestFiles(ctx, client, owner, repo, pullNumber, pagination)
return result, nil, err
case "get_review_comments":
result, err := GetPullRequestReviewComments(ctx, client, cache, owner, repo, pullNumber, pagination, flags)
gqlClient, err := getGQLClient(ctx)
if err != nil {
return utils.NewToolResultErrorFromErr("failed to get GitHub GQL client", err), nil, nil
}
result, err := GetPullRequestReviewComments(ctx, gqlClient, cache, owner, repo, pullNumber, pagination, flags)
return result, nil, err
case "get_reviews":
result, err := GetPullRequestReviews(ctx, client, cache, owner, repo, pullNumber, flags)
Expand Down Expand Up @@ -282,54 +286,130 @@ func GetPullRequestFiles(ctx context.Context, client *github.Client, owner, repo
return utils.NewToolResultText(string(r)), nil
}

func GetPullRequestReviewComments(ctx context.Context, client *github.Client, cache *lockdown.RepoAccessCache, owner, repo string, pullNumber int, pagination PaginationParams, ff FeatureFlags) (*mcp.CallToolResult, error) {
opts := &github.PullRequestListCommentsOptions{
ListOptions: github.ListOptions{
PerPage: pagination.PerPage,
Page: pagination.Page,
},
// GraphQL types for review threads query
type reviewThreadsQuery struct {
Repository struct {
PullRequest struct {
ReviewThreads struct {
Nodes []reviewThreadNode
PageInfo pageInfoFragment
TotalCount githubv4.Int
} `graphql:"reviewThreads(first: $first, after: $after)"`
} `graphql:"pullRequest(number: $prNum)"`
} `graphql:"repository(owner: $owner, name: $repo)"`
}

type reviewThreadNode struct {
ID githubv4.ID
IsResolved githubv4.Boolean
IsOutdated githubv4.Boolean
IsCollapsed githubv4.Boolean
Comments struct {
Nodes []reviewCommentNode
TotalCount githubv4.Int
} `graphql:"comments(first: $commentsPerThread)"`
}

type reviewCommentNode struct {
ID githubv4.ID
Body githubv4.String
Path githubv4.String
Line *githubv4.Int
Author struct {
Login githubv4.String
}
CreatedAt githubv4.DateTime
UpdatedAt githubv4.DateTime
URL githubv4.URI
}

type pageInfoFragment struct {
HasNextPage githubv4.Boolean
HasPreviousPage githubv4.Boolean
StartCursor githubv4.String
EndCursor githubv4.String
}

comments, resp, err := client.PullRequests.ListComments(ctx, owner, repo, pullNumber, opts)
func GetPullRequestReviewComments(ctx context.Context, gqlClient *githubv4.Client, cache *lockdown.RepoAccessCache, owner, repo string, pullNumber int, pagination PaginationParams, ff FeatureFlags) (*mcp.CallToolResult, error) {
// Convert pagination parameters to GraphQL format
gqlParams, err := pagination.ToGraphQLParams()
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx,
"failed to get pull request review comments",
resp,
err,
), nil
return utils.NewToolResultError(fmt.Sprintf("invalid pagination parameters: %v", err)), nil
}
defer func() { _ = resp.Body.Close() }()

if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
return utils.NewToolResultError(fmt.Sprintf("failed to get pull request review comments: %s", string(body))), nil
// Default to 100 threads if not specified, max is 100 for GraphQL
perPage := int32(100)
if gqlParams.First != nil && *gqlParams.First > 0 {
perPage = *gqlParams.First
}
Comment on lines +340 to 344
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "Default to 100 threads if not specified" is misleading. The gqlParams.First will always be non-nil and set to at least 30 (the default from OptionalPaginationParams), so this code path will never default to 100. The default is actually 30, not 100.

Consider either:

  1. Removing this comment and the default assignment (just use perPage := *gqlParams.First)
  2. Or updating the comment to clarify: "Use value from pagination params (default 30 via OptionalPaginationParams), max is 100 for GraphQL"
Suggested change
// Default to 100 threads if not specified, max is 100 for GraphQL
perPage := int32(100)
if gqlParams.First != nil && *gqlParams.First > 0 {
perPage = *gqlParams.First
}
// Use value from pagination params (default 30 via OptionalPaginationParams), max is 100 for GraphQL
perPage := *gqlParams.First

Copilot uses AI. Check for mistakes.

// Build variables for GraphQL query
vars := map[string]interface{}{
"owner": githubv4.String(owner),
"repo": githubv4.String(repo),
"prNum": githubv4.Int(int32(pullNumber)), //nolint:gosec // pullNumber is controlled by user input validation
"first": githubv4.Int(perPage),
"commentsPerThread": githubv4.Int(50), // Max 50 comments per thread
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded limit of 50 comments per thread may silently truncate threads with more comments. Consider documenting this limitation in the tool description or adding a field to the response indicating if comments were truncated.

For example, you could add to the thread response:

"commentsTruncated": len(thread.Comments.Nodes) >= 50

Or document in the tool description: "Note: Each thread returns at most 50 comments. Threads with more comments will be truncated."

Copilot uses AI. Check for mistakes.
}

// Add cursor if provided
if gqlParams.After != nil && *gqlParams.After != "" {
vars["after"] = githubv4.String(*gqlParams.After)
} else {
vars["after"] = (*githubv4.String)(nil)
}

// Execute GraphQL query
var query reviewThreadsQuery
if err := gqlClient.Query(ctx, &query, vars); err != nil {
return ghErrors.NewGitHubGraphQLErrorResponse(ctx,
"failed to get pull request review threads",
err,
), nil
}

// Lockdown mode filtering
if ff.LockdownMode {
if cache == nil {
return nil, fmt.Errorf("lockdown cache is not configured")
}
filteredComments := make([]*github.PullRequestComment, 0, len(comments))
for _, comment := range comments {
user := comment.GetUser()
if user == nil {
continue
}
isSafeContent, err := cache.IsSafeContent(ctx, user.GetLogin(), owner, repo)
if err != nil {
return utils.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil
}
if isSafeContent {
filteredComments = append(filteredComments, comment)

// Iterate through threads and filter comments
for i := range query.Repository.PullRequest.ReviewThreads.Nodes {
thread := &query.Repository.PullRequest.ReviewThreads.Nodes[i]
filteredComments := make([]reviewCommentNode, 0, len(thread.Comments.Nodes))

for _, comment := range thread.Comments.Nodes {
login := string(comment.Author.Login)
if login != "" {
isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo)
if err != nil {
return nil, fmt.Errorf("failed to check lockdown mode: %w", err)
}
if isSafeContent {
filteredComments = append(filteredComments, comment)
}
}
}

thread.Comments.Nodes = filteredComments
thread.Comments.TotalCount = githubv4.Int(int32(len(filteredComments))) //nolint:gosec // comment count is bounded by API limits
}
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After lockdown filtering, threads may end up with zero comments but are still included in the response. Consider filtering out empty threads after comment filtering to provide cleaner output:

// After filtering comments, remove threads with no remaining comments
filteredThreads := make([]reviewThreadNode, 0, len(query.Repository.PullRequest.ReviewThreads.Nodes))
for i := range query.Repository.PullRequest.ReviewThreads.Nodes {
    thread := &query.Repository.PullRequest.ReviewThreads.Nodes[i]
    if len(thread.Comments.Nodes) > 0 {
        filteredThreads = append(filteredThreads, *thread)
    }
}
query.Repository.PullRequest.ReviewThreads.Nodes = filteredThreads

This would be consistent with how GetPullRequestReviews filters out entire reviews in lockdown mode.

Suggested change
}
}
// After filtering comments, remove threads with no remaining comments
filteredThreads := make([]reviewThreadNode, 0, len(query.Repository.PullRequest.ReviewThreads.Nodes))
for i := range query.Repository.PullRequest.ReviewThreads.Nodes {
thread := &query.Repository.PullRequest.ReviewThreads.Nodes[i]
if len(thread.Comments.Nodes) > 0 {
filteredThreads = append(filteredThreads, *thread)
}
}
query.Repository.PullRequest.ReviewThreads.Nodes = filteredThreads

Copilot uses AI. Check for mistakes.
comments = filteredComments
}

r, err := json.Marshal(comments)
// Build response with review threads and pagination info
response := map[string]interface{}{
"reviewThreads": query.Repository.PullRequest.ReviewThreads.Nodes,
"pageInfo": map[string]interface{}{
"hasNextPage": query.Repository.PullRequest.ReviewThreads.PageInfo.HasNextPage,
"hasPreviousPage": query.Repository.PullRequest.ReviewThreads.PageInfo.HasPreviousPage,
"startCursor": string(query.Repository.PullRequest.ReviewThreads.PageInfo.StartCursor),
"endCursor": string(query.Repository.PullRequest.ReviewThreads.PageInfo.EndCursor),
},
"totalCount": int(query.Repository.PullRequest.ReviewThreads.TotalCount),
}
Comment on lines +401 to +410
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response structure has inconsistent JSON key casing. The top-level map uses camelCase keys ("reviewThreads", "pageInfo", "totalCount"), but the nested structs (reviewThreadNode, reviewCommentNode, pageInfoFragment) don't have JSON tags, so they'll marshal to PascalCase by default.

This creates inconsistent output like:

{
  "reviewThreads": [
    {
      "ID": "...",              // PascalCase from struct
      "IsResolved": false,      // PascalCase from struct
      ...
    }
  ],
  "pageInfo": { ... },          // camelCase from map key
  "totalCount": 1               // camelCase from map key
}

Consider adding JSON tags to the struct fields to maintain consistent camelCase throughout:

type reviewThreadNode struct {
    ID          githubv4.ID      `graphql:"id" json:"id"`
    IsResolved  githubv4.Boolean `graphql:"isResolved" json:"isResolved"`
    // ... etc
}

Copilot uses AI. Check for mistakes.

r, err := json.Marshal(response)
if err != nil {
return nil, fmt.Errorf("failed to marshal response: %w", err)
}
Expand Down
Loading