diff --git a/README.md b/README.md index d013303d81..bb1e3b83ef 100644 --- a/README.md +++ b/README.md @@ -1274,6 +1274,7 @@ The following sets of tools are available: - **pull_request_read** - Get details for a single pull request - **OAuth Challenge Scopes**: `repo` - `after`: Cursor for pagination, used only by the get_review_comments method. Pass the endCursor from the previous page's PageInfo to fetch the next page. (string, optional) + - `fields`: Subset of fields to return for pull_request_read results. Supported for get_reviews and get_check_runs. Valid fields depend on the selected method. If omitted or empty, all fields are returned. (string[], optional) - `method`: Action to specify what pull request data needs to be retrieved from GitHub. Possible options: 1. get - Get details of a specific pull request. diff --git a/pkg/github/__toolsnaps__/pull_request_read.snap b/pkg/github/__toolsnaps__/pull_request_read.snap index d518c7cad9..ae591da69a 100644 --- a/pkg/github/__toolsnaps__/pull_request_read.snap +++ b/pkg/github/__toolsnaps__/pull_request_read.snap @@ -11,6 +11,29 @@ "description": "Cursor for pagination, used only by the get_review_comments method. Pass the endCursor from the previous page's PageInfo to fetch the next page.", "type": "string" }, + "fields": { + "description": "Subset of fields to return for pull_request_read results. Supported for get_reviews and get_check_runs. Valid fields depend on the selected method. If omitted or empty, all fields are returned.", + "items": { + "enum": [ + "id", + "state", + "body", + "html_url", + "user", + "commit_id", + "submitted_at", + "author_association", + "name", + "status", + "conclusion", + "details_url", + "started_at", + "completed_at" + ], + "type": "string" + }, + "type": "array" + }, "method": { "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 combined commit status of a head commit in a pull request.\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_commits - Get the list of commits on a pull request. Use with pagination parameters to control the number of results returned.\n 6. 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 thread metadata and comments with nullable current and original line-range coordinates (line, start_line, original_line, original_start_line). Current coordinates are omitted when unavailable, such as for outdated comments. Use cursor-based pagination (perPage, after) to control results.\n 7. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.\n 8. 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 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.\n", "enum": [ diff --git a/pkg/github/fields_filtering_test.go b/pkg/github/fields_filtering_test.go index cd08dcef68..0f0129c112 100644 --- a/pkg/github/fields_filtering_test.go +++ b/pkg/github/fields_filtering_test.go @@ -3,13 +3,16 @@ package github import ( "context" "encoding/json" + "maps" "net/http" "testing" + "time" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -513,3 +516,460 @@ func assertFieldsTelemetry(t *testing.T, serverTool inventory.ServerTool, client assert.False(t, ok, "no byte counters when not filtered") }) } + +// --- pull_request_read ---------------------------------------------------- + +func Test_PullRequestRead_FieldsSchema(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + schema := serverTool.Tool.InputSchema.(*jsonschema.Schema) + + fields, ok := schema.Properties["fields"] + require.True(t, ok) + require.Equal(t, "array", fields.Type) + require.NotNil(t, fields.Items) + + assert.Contains(t, fields.Items.Enum, "state") + assert.Contains(t, fields.Items.Enum, "user") + assert.Contains(t, fields.Items.Enum, "body") + + assert.Contains(t, fields.Items.Enum, "name") + assert.Contains(t, fields.Items.Enum, "conclusion") + assert.Contains(t, fields.Items.Enum, "details_url") +} + +func Test_PullRequestRead_FieldsRejectUnsupportedMethod(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil)), + } + + request := createMCPRequest(map[string]any{ + "method": "get", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "fields": []any{"state"}, + }) + + result, err := serverTool.Handler(deps)( + ContextWithDeps(context.Background(), deps), + &request, + ) + + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains( + t, + getErrorResult(t, result).Text, + `fields is not supported for pull_request_read method "get"`, + ) +} + +func Test_PullRequestRead_FieldsRejectFieldForWrongMethod(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil)), + } + + request := createMCPRequest(map[string]any{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "fields": []any{"name"}, + }) + + result, err := serverTool.Handler(deps)( + ContextWithDeps(context.Background(), deps), + &request, + ) + + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains( + t, + getErrorResult(t, result).Text, + `field "name" is not supported for pull_request_read method "get_reviews"`, + ) +} + +func mockPullRequestReviews() []*github.PullRequestReview { + return []*github.PullRequestReview{ + { + ID: github.Ptr(int64(101)), + State: github.Ptr("APPROVED"), + Body: github.Ptr( + "large review body that should disappear when fields are selected", + ), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42#pullrequestreview-101"), + User: &github.User{ + Login: github.Ptr("reviewer"), + }, + CommitID: github.Ptr("abcdef123456"), + SubmittedAt: &github.Timestamp{Time: time.Date(2024, 1, 2, 3, 4, 5, 0, time.UTC)}, + }, + } +} + +func Test_PullRequestRead_GetReviews_Fields(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestReviews()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + call := func(t *testing.T, args map[string]any) string { + t.Helper() + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + if result.IsError { + t.Fatalf("unexpected tool error: %s", getErrorResult(t, result).Text) + } + return getTextResult(t, result).Text + } + + baseArgs := map[string]any{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + } + + t.Run("selected fields filter each review", func(t *testing.T) { + args := map[string]any{} + maps.Copy(args, baseArgs) + args["fields"] = []any{"state", "user"} + + text := call(t, args) + + var returned []map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + require.Len(t, returned, 1) + require.Len(t, returned[0], 2) + + assert.Equal(t, "APPROVED", returned[0]["state"]) + assert.Contains(t, returned[0], "user") + assert.NotContains(t, returned[0], "body") + assert.NotContains(t, returned[0], "id") + }) + + t.Run("omitted fields keeps the full response", func(t *testing.T) { + text := call(t, baseArgs) + + assert.Contains(t, text, `"body"`) + assert.Contains(t, text, "large review body that should disappear when fields are selected") + + var returned []map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + require.Len(t, returned, 1) + assert.Contains(t, returned[0], "id") + assert.Contains(t, returned[0], "state") + assert.Contains(t, returned[0], "body") + assert.Contains(t, returned[0], "user") + }) + + t.Run("empty fields keeps the full response", func(t *testing.T) { + args := map[string]any{} + maps.Copy(args, baseArgs) + args["fields"] = []any{} + + text := call(t, args) + + assert.Contains(t, text, `"body"`) + + var returned []map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + require.Len(t, returned, 1) + assert.Contains(t, returned[0], "id") + assert.Contains(t, returned[0], "body") + }) +} + +func mockPullRequestForCheckRuns() *github.PullRequest { + return &github.PullRequest{ + Number: github.Ptr(42), + Head: &github.PullRequestBranch{ + SHA: github.Ptr("abcd1234"), + Ref: github.Ptr("feature-branch"), + }, + } +} + +func mockCheckRuns() *github.ListCheckRunsResults { + return &github.ListCheckRunsResults{ + Total: github.Ptr(2), + CheckRuns: []*github.CheckRun{ + { + ID: github.Ptr(int64(1)), + Name: github.Ptr("test"), + Status: github.Ptr("completed"), + Conclusion: github.Ptr("success"), + DetailsURL: github.Ptr("https://example.test/test"), + }, + { + ID: github.Ptr(int64(2)), + Name: github.Ptr("lint"), + Status: github.Ptr("completed"), + Conclusion: github.Ptr("failure"), + DetailsURL: github.Ptr("https://example.test/lint"), + }, + }, + } +} + +func Test_PullRequestRead_GetCheckRuns_Fields(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestForCheckRuns()), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCheckRuns()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + call := func(t *testing.T, args map[string]any) string { + t.Helper() + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + if result.IsError { + t.Fatalf("unexpected tool error: %s", getErrorResult(t, result).Text) + } + return getTextResult(t, result).Text + } + + baseArgs := map[string]any{ + "method": "get_check_runs", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + } + + type checkRunsResult struct { + TotalCount int `json:"total_count"` + CheckRuns []map[string]any `json:"check_runs"` + } + + t.Run("selected fields filter each check run and preserve the wrapper", func(t *testing.T) { + args := map[string]any{} + maps.Copy(args, baseArgs) + args["fields"] = []any{"name", "conclusion"} + + text := call(t, args) + + var returned checkRunsResult + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + assert.Equal(t, 2, returned.TotalCount) + require.Len(t, returned.CheckRuns, 2) + + require.Len(t, returned.CheckRuns[0], 2) + assert.Equal(t, "test", returned.CheckRuns[0]["name"]) + assert.Equal(t, "success", returned.CheckRuns[0]["conclusion"]) + assert.NotContains(t, returned.CheckRuns[0], "id") + assert.NotContains(t, returned.CheckRuns[0], "details_url") + + assert.Equal(t, "lint", returned.CheckRuns[1]["name"]) + assert.Equal(t, "failure", returned.CheckRuns[1]["conclusion"]) + }) + + t.Run("omitted fields keeps the full response", func(t *testing.T) { + text := call(t, baseArgs) + + var returned checkRunsResult + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + assert.Equal(t, 2, returned.TotalCount) + require.Len(t, returned.CheckRuns, 2) + assert.Contains(t, returned.CheckRuns[0], "id") + assert.Contains(t, returned.CheckRuns[0], "details_url") + }) + + t.Run("empty fields keeps the full response", func(t *testing.T) { + args := map[string]any{} + maps.Copy(args, baseArgs) + args["fields"] = []any{} + + text := call(t, args) + + var returned checkRunsResult + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + assert.Equal(t, 2, returned.TotalCount) + require.Len(t, returned.CheckRuns, 2) + assert.Contains(t, returned.CheckRuns[0], "id") + assert.Contains(t, returned.CheckRuns[0], "details_url") + }) +} + +func Test_PullRequestRead_FieldsTelemetry(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + + t.Run("get_reviews", func(t *testing.T) { + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestReviews()), + })) + + base := map[string]any{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + } + + filtered := map[string]any{} + maps.Copy(filtered, base) + filtered["fields"] = []any{"state"} + + assertFieldsTelemetry(t, serverTool, client, "pull_request_read", filtered, base) + }) + + t.Run("get_check_runs", func(t *testing.T) { + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestForCheckRuns()), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCheckRuns()), + })) + + base := map[string]any{ + "method": "get_check_runs", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + } + + filtered := map[string]any{} + maps.Copy(filtered, base) + filtered["fields"] = []any{"name"} + + assertFieldsTelemetry(t, serverTool, client, "pull_request_read", filtered, base) + }) +} + +// Test_PullRequestRead_FieldsPreserveIFCLabel guards the constraint that +// response field filtering must not change IFC label behavior: the label is +// attached by the consolidated handler after the getter returns, so filtering +// the payload must leave it intact. +func Test_PullRequestRead_FieldsPreserveIFCLabel(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + + t.Run("get_reviews", func(t *testing.T) { + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestReviews()), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{ + "name": "repo", + "private": false, + }), + })), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "fields": []any{"state"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + require.NotNil(t, result.Meta) + ifcMap := unmarshalIFC(t, result.Meta["ifc"]) + assert.Equal(t, "untrusted", ifcMap["integrity"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) + }) + + t.Run("get_check_runs", func(t *testing.T) { + deps := BaseDeps{ + Client: mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPullRequestForCheckRuns()), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCheckRuns()), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, map[string]any{ + "name": "repo", + "private": false, + }), + })), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get_check_runs", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "fields": []any{"name"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + require.NotNil(t, result.Meta) + ifcMap := unmarshalIFC(t, result.Meta["ifc"]) + assert.Equal(t, "untrusted", ifcMap["integrity"]) + assert.Equal(t, "public", ifcMap["confidentiality"]) + }) +} + +// Test_PullRequestRead_FieldsAfterLockdown proves that lockdown/security +// filtering and response field filtering compose in the required order on a +// single call: disallowed reviews are dropped first, then the surviving review +// is trimmed to the requested fields. +func Test_PullRequestRead_FieldsAfterLockdown(t *testing.T) { + serverTool := PullRequestRead(translations.NullTranslationHelper) + + reviews := []*github.PullRequestReview{ + { + ID: github.Ptr(int64(2030)), + State: github.Ptr("APPROVED"), + Body: github.Ptr("Maintainer review"), + User: &github.User{Login: github.Ptr("maintainer")}, + }, + { + ID: github.Ptr(int64(2031)), + State: github.Ptr("COMMENTED"), + Body: github.Ptr("External reviewer"), + User: &github.User{Login: github.Ptr("testuser")}, + }, + } + + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, reviews), + })) + restClient := mockRESTPermissionServer(t, "read", map[string]string{ + "maintainer": "write", + "testuser": "read", + }) + + deps := BaseDeps{ + Client: client, + RepoAccessCache: stubRepoAccessCache(restClient, 5*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": true}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get_reviews", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "fields": []any{"state"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + var returned []map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + + // Lockdown dropped the external reviewer (read-only permission). + require.Len(t, returned, 1) + // Field filtering then kept only `state` on the survivor. + require.Len(t, returned[0], 1) + assert.Equal(t, "APPROVED", returned[0]["state"]) + assert.NotContains(t, returned[0], "body") + assert.NotContains(t, returned[0], "id") +} diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index 2eba9a1628..042b0ca1fc 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -93,6 +93,49 @@ var searchPullRequestsItemFieldEnum = []any{ "closed_by", "pull_request", "repository_url", } +// pullRequestReviewItemFieldEnum lists the selectable fields for +// pull_request_read get_reviews result items. Review bodies can be large, so +// omitting them is the main lever for shrinking review listings. +var pullRequestReviewItemFieldEnum = []any{ + "id", "state", "body", "html_url", "user", "commit_id", "submitted_at", + "author_association", +} + +// pullRequestCheckRunItemFieldEnum lists the selectable fields for +// pull_request_read get_check_runs result items. +var pullRequestCheckRunItemFieldEnum = []any{ + "id", "name", "status", "conclusion", "html_url", "details_url", + "started_at", "completed_at", +} + +// pullRequestReadItemFieldEnum is the union of the per-method enums, exposed on +// the consolidated pull_request_read schema. The union admits every field any +// supported method can return; validatePullRequestReadFields narrows the +// selection to the fields valid for the selected method at runtime. Deriving it +// from the per-method enums keeps the schema from drifting out of sync with the +// runtime validator, which would otherwise silently make a valid field +// unreachable. +var pullRequestReadItemFieldEnum = unionFieldEnums( + pullRequestReviewItemFieldEnum, + pullRequestCheckRunItemFieldEnum, +) + +// unionFieldEnums returns the ordered, de-duplicated union of the given enums. +func unionFieldEnums(enums ...[]any) []any { + seen := make(map[any]struct{}) + union := make([]any, 0) + for _, enum := range enums { + for _, field := range enum { + if _, ok := seen[field]; ok { + continue + } + seen[field] = struct{}{} + union = append(union, field) + } + } + return union +} + // filterFields marshals v to a JSON object and returns a map containing only the // requested fields. Fields that are unknown or absent from the JSON (for example // empty values dropped via omitempty) are skipped. diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 5cee8b3231..23b900c574 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "slices" "github.com/go-viper/mapstructure/v2" "github.com/google/go-github/v89/github" @@ -22,6 +23,44 @@ import ( "github.com/github/github-mcp-server/pkg/utils" ) +// pullRequestReadFieldsByMethod maps each fields-enabled pull_request_read +// method to the field names it may return. The consolidated schema exposes the +// union of these; this table is what makes the selection method-aware. +var pullRequestReadFieldsByMethod = map[string][]any{ + "get_reviews": pullRequestReviewItemFieldEnum, + "get_check_runs": pullRequestCheckRunItemFieldEnum, +} + +// validatePullRequestReadFields rejects a non-empty fields selection that is not +// supported by the selected method. An empty selection is always allowed (it is +// equivalent to omitting the parameter) and every method keeps returning its +// full response. +func validatePullRequestReadFields(method string, fields []string) error { + if len(fields) == 0 { + return nil + } + + allowed, ok := pullRequestReadFieldsByMethod[method] + if !ok { + return fmt.Errorf( + "fields is not supported for pull_request_read method %q", + method, + ) + } + + for _, field := range fields { + if !slices.Contains(allowed, any(field)) { + return fmt.Errorf( + "field %q is not supported for pull_request_read method %q", + field, + method, + ) + } + } + + return nil +} + // PullRequestRead creates a tool to get details of a specific pull request. func PullRequestRead(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ @@ -66,6 +105,13 @@ Possible options: Type: "string", Description: "Cursor for pagination, used only by the get_review_comments method. Pass the endCursor from the previous page's PageInfo to fetch the next page.", } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for pull_request_read results. "+ + "Supported for get_reviews and get_check_runs. "+ + "Valid fields depend on the selected method. "+ + "If omitted or empty, all fields are returned.", + pullRequestReadItemFieldEnum, + ) return NewTool( ToolsetMetadataPullRequests, @@ -97,6 +143,15 @@ Possible options: if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + // Validate before any GitHub API call so an invalid method/field + // combination fails fast without a network round-trip. + if err := validatePullRequestReadFields(method, fields); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -145,13 +200,13 @@ Possible options: result, err := GetPullRequestReviewComments(ctx, gqlClient, deps, owner, repo, pullNumber, cursorPagination) return attachIFC(result), nil, err case "get_reviews": - result, err := GetPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination) + result, err := GetPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination, fields) return attachIFC(result), nil, err case "get_comments": result, err := GetIssueComments(ctx, client, deps, owner, repo, pullNumber, pagination) return attachIFC(result), nil, err case "get_check_runs": - result, err := GetPullRequestCheckRuns(ctx, client, owner, repo, pullNumber, pagination) + result, err := GetPullRequestCheckRuns(ctx, client, deps, owner, repo, pullNumber, pagination, fields) return attachIFC(result), nil, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil @@ -304,7 +359,7 @@ func GetPullRequestStatus(ctx context.Context, client *github.Client, owner, rep return utils.NewToolResultText(string(r)), nil } -func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { +func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams, fields []string) (*mcp.CallToolResult, error) { // First get the PR to get the head SHA pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) if err != nil { @@ -361,11 +416,49 @@ func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, CheckRuns: minimalCheckRuns, } - r, err := json.Marshal(minimalResult) + // Filter only the check_run items: total_count is response metadata, not an + // item field, and must survive filtering so callers can still reason about + // the unpaginated total. An empty selection keeps the full response. + filtered := false + var payload any = minimalResult + + if len(fields) > 0 { + filteredCheckRuns, err := filterEachField(minimalCheckRuns, fields) + if err != nil { + return utils.NewToolResultErrorFromErr( + "failed to filter pull request check runs", + err, + ), nil + } + + payload = struct { + TotalCount int `json:"total_count"` + CheckRuns []map[string]any `json:"check_runs"` + }{ + TotalCount: minimalResult.TotalCount, + CheckRuns: filteredCheckRuns, + } + + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) + return utils.NewToolResultErrorFromErr( + "failed to marshal pull request check runs", + err, + ), nil } + recordFieldsUsageFor( + ctx, + deps, + "pull_request_read", + minimalResult, + filtered, + len(r), + ) + return utils.NewToolResultText(string(r)), nil } @@ -553,7 +646,7 @@ func GetPullRequestReviewComments(ctx context.Context, gqlClient *githubv4.Clien return MarshalledTextResult(convertToMinimalReviewThreadsResponse(query)), nil } -func GetPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { +func GetPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams, fields []string) (*mcp.CallToolResult, error) { cache, err := deps.GetRepoAccessCache(ctx) if err != nil { return nil, fmt.Errorf("failed to get repo access cache: %w", err) @@ -607,7 +700,43 @@ func GetPullRequestReviews(ctx context.Context, client *github.Client, deps Tool minimalReviews = append(minimalReviews, convertToMinimalPullRequestReview(review)) } - return MarshalledTextResult(minimalReviews), nil + // Field filtering is applied after lockdown/security filtering and after the + // API payload has been reduced to MinimalPullRequestReview values. An empty + // selection keeps the full response. + filtered := false + var payload any = minimalReviews + + if len(fields) > 0 { + filteredReviews, err := filterEachField(minimalReviews, fields) + if err != nil { + return utils.NewToolResultErrorFromErr( + "failed to filter pull request reviews", + err, + ), nil + } + + payload = filteredReviews + filtered = true + } + + r, err := json.Marshal(payload) + if err != nil { + return utils.NewToolResultErrorFromErr( + "failed to marshal pull request reviews", + err, + ), nil + } + + recordFieldsUsageFor( + ctx, + deps, + "pull_request_read", + minimalReviews, + filtered, + len(r), + ) + + return utils.NewToolResultText(string(r)), nil } // PullRequestWriteUIResourceURI is the URI for the create_pull_request tool's MCP App UI resource.