diff --git a/README.md b/README.md index 6413b58760..a7d7d05270 100644 --- a/README.md +++ b/README.md @@ -1313,6 +1313,7 @@ For negotiated supported protocol versions `2026-07-28` or newer, the consolidat - **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 c99d4276e2..58ee26c05e 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": [ @@ -581,7 +604,10 @@ "properties": { "completed_at": { "description": "RFC 3339 timestamp.", - "type": "string" + "type": [ + "null", + "string" + ] }, "conclusion": { "description": "Completed check outcome.", @@ -596,24 +622,42 @@ "stale", "startup_failure" ], - "type": "string" + "type": [ + "null", + "string" + ] }, "details_url": { - "type": "string" + "type": [ + "null", + "string" + ] }, "html_url": { "description": "Browser URL.", - "type": "string" + "type": [ + "null", + "string" + ] }, "id": { - "type": "integer" + "type": [ + "null", + "integer" + ] }, "name": { - "type": "string" + "type": [ + "null", + "string" + ] }, "started_at": { "description": "RFC 3339 timestamp.", - "type": "string" + "type": [ + "null", + "string" + ] }, "status": { "description": "Check execution status; empty for a sparse response.", @@ -626,14 +670,12 @@ "pending", "" ], - "type": "string" + "type": [ + "null", + "string" + ] } }, - "required": [ - "id", - "name", - "status" - ], "type": "object" }, "type": [ @@ -752,21 +794,36 @@ "additionalProperties": false, "properties": { "author_association": { - "type": "string" + "type": [ + "null", + "string" + ] }, "body": { - "type": "string" + "type": [ + "null", + "string" + ] }, "commit_id": { "description": "Git commit SHA.", - "type": "string" + "type": [ + "null", + "string" + ] }, "html_url": { "description": "Browser URL.", - "type": "string" + "type": [ + "null", + "string" + ] }, "id": { - "type": "integer" + "type": [ + "null", + "integer" + ] }, "state": { "description": "Review state; empty for a sparse response.", @@ -778,11 +835,17 @@ "PENDING", "" ], - "type": "string" + "type": [ + "null", + "string" + ] }, "submitted_at": { "description": "RFC 3339 timestamp.", - "type": "string" + "type": [ + "null", + "string" + ] }, "user": { "additionalProperties": false, @@ -879,11 +942,6 @@ ] } }, - "required": [ - "id", - "state", - "html_url" - ], "type": "object" }, "type": [ diff --git a/pkg/github/consolidated_pull_request_types.go b/pkg/github/consolidated_pull_request_types.go index 6d6abaed63..d0720a1936 100644 --- a/pkg/github/consolidated_pull_request_types.go +++ b/pkg/github/consolidated_pull_request_types.go @@ -14,13 +14,14 @@ import ( ) type PullRequestReadInput struct { - Method string `json:"method"` - Owner string `json:"owner"` - Repo string `json:"repo"` - PullNumber int `json:"pullNumber"` - Page *int `json:"page,omitempty"` - PerPage *int `json:"perPage,omitempty"` - After *string `json:"after,omitempty"` + Method string `json:"method"` + Owner string `json:"owner"` + Repo string `json:"repo"` + PullNumber int `json:"pullNumber"` + Fields []string `json:"fields,omitempty"` + Page *int `json:"page,omitempty"` + PerPage *int `json:"perPage,omitempty"` + After *string `json:"after,omitempty"` } type CreatePullRequestInput struct { @@ -114,9 +115,9 @@ type PullRequestReadOutput struct { Files *[]MinimalPRFile Commits *[]MinimalPullRequestCommit ReviewThreads *MinimalReviewThreadsResponse - Reviews *[]MinimalPullRequestReview + Reviews *[]PullRequestReviewOutput Comments *[]IssueReadCommentOutput - CheckRuns *MinimalCheckRunsResult + CheckRuns *PullRequestCheckRunsOutput } // PullRequestDiffOutput wraps the raw diff, which the text content returns verbatim. @@ -151,6 +152,125 @@ func (out PullRequestReadOutput) MarshalJSON() ([]byte, error) { } } +// fieldSelector reports whether a field should be emitted for the given +// selection. An empty selection keeps every field, matching the behaviour of +// omitting the `fields` parameter entirely. +func fieldSelector(fields []string) func(string) bool { + return func(field string) bool { + return len(fields) == 0 || slices.Contains(fields, field) + } +} + +// PullRequestReviewOutput is the field-projected shape of a single review. Every +// field is optional so a `fields` selection can omit anything the caller did not +// ask for while still satisfying the typed output schema. The unfiltered case +// populates every populated field and so matches the historical JSON exactly. +type PullRequestReviewOutput struct { + ID *int64 `json:"id,omitempty"` + State *string `json:"state,omitempty"` + Body *string `json:"body,omitempty"` + HTMLURL *string `json:"html_url,omitempty"` + User *MinimalUser `json:"user,omitempty"` + CommitID *string `json:"commit_id,omitempty"` + SubmittedAt *string `json:"submitted_at,omitempty"` + AuthorAssociation *string `json:"author_association,omitempty"` +} + +// structuredPullRequestReviewsOutput projects converted reviews onto the selected +// fields so structuredContent matches the filtered text response. +func structuredPullRequestReviewsOutput(reviews []MinimalPullRequestReview, fields []string) []PullRequestReviewOutput { + selected := fieldSelector(fields) + output := make([]PullRequestReviewOutput, 0, len(reviews)) + for _, review := range reviews { + item := PullRequestReviewOutput{} + if selected("id") { + item.ID = new(review.ID) + } + if selected("state") { + item.State = new(review.State) + } + if selected("body") && review.Body != "" { + item.Body = new(review.Body) + } + if selected("html_url") { + item.HTMLURL = new(review.HTMLURL) + } + if selected("user") && review.User != nil { + item.User = review.User + } + if selected("commit_id") && review.CommitID != "" { + item.CommitID = new(review.CommitID) + } + if selected("submitted_at") && review.SubmittedAt != "" { + item.SubmittedAt = new(review.SubmittedAt) + } + if selected("author_association") && review.AuthorAssociation != "" { + item.AuthorAssociation = new(review.AuthorAssociation) + } + output = append(output, item) + } + return output +} + +// PullRequestCheckRunOutput is the field-projected shape of a single check run. +type PullRequestCheckRunOutput struct { + ID *int64 `json:"id,omitempty"` + Name *string `json:"name,omitempty"` + Status *string `json:"status,omitempty"` + Conclusion *string `json:"conclusion,omitempty"` + HTMLURL *string `json:"html_url,omitempty"` + DetailsURL *string `json:"details_url,omitempty"` + StartedAt *string `json:"started_at,omitempty"` + CompletedAt *string `json:"completed_at,omitempty"` +} + +// PullRequestCheckRunsOutput keeps the total_count wrapper. total_count is +// response metadata rather than an item field, so a `fields` selection never +// removes it. +type PullRequestCheckRunsOutput struct { + TotalCount int `json:"total_count"` + CheckRuns []PullRequestCheckRunOutput `json:"check_runs"` +} + +// structuredPullRequestCheckRunsOutput projects check runs onto the selected +// fields while always preserving total_count. +func structuredPullRequestCheckRunsOutput(result MinimalCheckRunsResult, fields []string) PullRequestCheckRunsOutput { + selected := fieldSelector(fields) + output := PullRequestCheckRunsOutput{ + TotalCount: result.TotalCount, + CheckRuns: make([]PullRequestCheckRunOutput, 0, len(result.CheckRuns)), + } + for _, run := range result.CheckRuns { + item := PullRequestCheckRunOutput{} + if selected("id") { + item.ID = new(run.ID) + } + if selected("name") { + item.Name = new(run.Name) + } + if selected("status") { + item.Status = new(run.Status) + } + if selected("conclusion") && run.Conclusion != "" { + item.Conclusion = new(run.Conclusion) + } + if selected("html_url") && run.HTMLURL != "" { + item.HTMLURL = new(run.HTMLURL) + } + if selected("details_url") && run.DetailsURL != "" { + item.DetailsURL = new(run.DetailsURL) + } + if selected("started_at") && run.StartedAt != "" { + item.StartedAt = new(run.StartedAt) + } + if selected("completed_at") && run.CompletedAt != "" { + item.CompletedAt = new(run.CompletedAt) + } + output.CheckRuns = append(output.CheckRuns, item) + } + return output +} + func pullRequestCommentsOutput(output *IssueReadOutput) *PullRequestReadOutput { if output == nil || output.Comments == nil { return nil @@ -162,7 +282,7 @@ func pullRequestReadOutputSchema() *jsonschema.Schema { arrays := []*jsonschema.Schema{ pullRequestOutputSchema[[]MinimalPRFile](), pullRequestOutputSchema[[]MinimalPullRequestCommit](), - pullRequestOutputSchema[[]MinimalPullRequestReview](), + pullRequestOutputSchema[[]PullRequestReviewOutput](), pullRequestOutputSchema[[]IssueReadCommentOutput](), } for _, array := range arrays { @@ -176,7 +296,7 @@ func pullRequestReadOutputSchema() *jsonschema.Schema { pullRequestOutputSchema[PullRequestDiffOutput](), pullRequestOutputSchema[MinimalCombinedStatus](), pullRequestOutputSchema[MinimalReviewThreadsResponse](), - pullRequestOutputSchema[MinimalCheckRunsResult](), + pullRequestOutputSchema[PullRequestCheckRunsOutput](), &jsonschema.Schema{Type: "array", AnyOf: arrays}, ) } diff --git a/pkg/github/fields_filtering_test.go b/pkg/github/fields_filtering_test.go index b54480252f..51c75f4ea8 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/v2/internal/githubv4mock" "github.com/github/github-mcp-server/v2/pkg/inventory" "github.com/github/github-mcp-server/v2/pkg/translations" "github.com/google/go-github/v92/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: new(int64(101)), + State: new("APPROVED"), + Body: new( + "large review body that should disappear when fields are selected", + ), + HTMLURL: new("https://github.com/owner/repo/pull/42#pullrequestreview-101"), + User: &github.User{ + Login: new("reviewer"), + }, + CommitID: new("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: new(42), + Head: &github.PullRequestBranch{ + SHA: new("abcd1234"), + Ref: new("feature-branch"), + }, + } +} + +func mockCheckRuns() *github.ListCheckRunsResults { + return &github.ListCheckRunsResults{ + Total: new(2), + CheckRuns: []*github.CheckRun{ + { + ID: new(int64(1)), + Name: new("test"), + Status: new("completed"), + Conclusion: new("success"), + DetailsURL: new("https://example.test/test"), + }, + { + ID: new(int64(2)), + Name: new("lint"), + Status: new("completed"), + Conclusion: new("failure"), + DetailsURL: new("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: new(int64(2030)), + State: new("APPROVED"), + Body: new("Maintainer review"), + User: &github.User{Login: new("maintainer")}, + }, + { + ID: new(int64(2031)), + State: new("COMMENTED"), + Body: new("External reviewer"), + User: &github.User{Login: new("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 b005223cd3..9cfc1957bf 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 5545d43964..e8b5e91ca3 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -23,6 +23,44 @@ import ( "github.com/github/github-mcp-server/v2/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{ @@ -67,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[PullRequestReadInput, *PullRequestReadOutput]( ToolsetMetadataPullRequests, @@ -103,6 +148,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 @@ -151,13 +205,13 @@ Possible options: result, output, err := getPullRequestReviewComments(ctx, gqlClient, deps, owner, repo, pullNumber, cursorPagination) return attachIFC(result), output, err case "get_reviews": - result, output, err := getPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination) + result, output, err := getPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination, fields) return attachIFC(result), output, err case "get_comments": result, output, err := getIssueComments(ctx, client, deps, owner, repo, pullNumber, pagination) return attachIFC(result), pullRequestCommentsOutput(output), err case "get_check_runs": - result, output, err := getPullRequestCheckRuns(ctx, client, owner, repo, pullNumber, pagination) + result, output, err := getPullRequestCheckRuns(ctx, client, deps, owner, repo, pullNumber, pagination, fields) return attachIFC(result), output, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil @@ -330,12 +384,12 @@ func getPullRequestStatus(ctx context.Context, client *github.Client, owner, rep return utils.NewToolResultText(string(r)), &PullRequestReadOutput{Status: &minimalStatus}, nil } -func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { - result, _, err := getPullRequestCheckRuns(ctx, client, owner, repo, pullNumber, pagination) +func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { + result, _, err := getPullRequestCheckRuns(ctx, client, deps, owner, repo, pullNumber, pagination, nil) return result, err } -func getPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, *PullRequestReadOutput, error) { +func getPullRequestCheckRuns(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams, fields []string) (*mcp.CallToolResult, *PullRequestReadOutput, error) { // First get the PR to get the head SHA pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) if err != nil { @@ -392,12 +446,54 @@ 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, 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, nil, fmt.Errorf("failed to marshal response: %w", err) + return utils.NewToolResultErrorFromErr( + "failed to marshal pull request check runs", + err, + ), nil, nil } - return utils.NewToolResultText(string(r)), &PullRequestReadOutput{CheckRuns: &minimalResult}, nil + recordFieldsUsageFor( + ctx, + deps, + "pull_request_read", + minimalResult, + filtered, + len(r), + ) + + // structuredContent must match the filtered text response, so project the + // check runs onto the same selected fields. total_count is preserved. + structured := structuredPullRequestCheckRunsOutput(minimalResult, fields) + + return utils.NewToolResultText(string(r)), &PullRequestReadOutput{CheckRuns: &structured}, nil } func GetPullRequestFiles(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { @@ -601,11 +697,11 @@ func getPullRequestReviewComments(ctx context.Context, gqlClient *githubv4.Clien } func GetPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { - result, _, err := getPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination) + result, _, err := getPullRequestReviews(ctx, client, deps, owner, repo, pullNumber, pagination, nil) return result, err } -func getPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, *PullRequestReadOutput, error) { +func getPullRequestReviews(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams, fields []string) (*mcp.CallToolResult, *PullRequestReadOutput, error) { cache, err := deps.GetRepoAccessCache(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get repo access cache: %w", err) @@ -659,7 +755,47 @@ func getPullRequestReviews(ctx context.Context, client *github.Client, deps Tool minimalReviews = append(minimalReviews, convertToMinimalPullRequestReview(review)) } - return MarshalledTextResult(minimalReviews), &PullRequestReadOutput{Reviews: &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, nil + } + + payload = filteredReviews + filtered = true + } + + r, err := json.Marshal(payload) + if err != nil { + return utils.NewToolResultErrorFromErr( + "failed to marshal pull request reviews", + err, + ), nil, nil + } + + recordFieldsUsageFor( + ctx, + deps, + "pull_request_read", + minimalReviews, + filtered, + len(r), + ) + + // structuredContent must match the filtered text response, so project the + // reviews onto the same selected fields. + structured := structuredPullRequestReviewsOutput(minimalReviews, fields) + + return utils.NewToolResultText(string(r)), &PullRequestReadOutput{Reviews: &structured}, nil } // PullRequestWriteUIResourceURI is the URI for the create_pull_request tool's MCP App UI resource. diff --git a/pkg/github/typed_consolidated_pull_request_outputs_test.go b/pkg/github/typed_consolidated_pull_request_outputs_test.go index 60afd2f1f1..1198721e32 100644 --- a/pkg/github/typed_consolidated_pull_request_outputs_test.go +++ b/pkg/github/typed_consolidated_pull_request_outputs_test.go @@ -288,10 +288,19 @@ func TestTypedConsolidatedPullRequestOutputs(t *testing.T) { {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 1}, `[{"id":9,"state":"APPROVED","body":"lgtm","html_url":"https://github.com/owner/repo/pull/1#pullrequestreview-9","user":{"login":"octocat"},"commit_id":"abc","author_association":"MEMBER"}]`}, {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 2}, `[]`}, {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 3}, `[]`}, + // `fields` projects the response for both the text content and the + // structured content; runTypedPullRequestCases asserts they stay + // byte-for-byte equivalent JSON and validates against the output schema. + {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 1, "fields": []any{"state", "user"}}, `[{"state":"APPROVED","user":{"login":"octocat"}}]`}, + {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 1, "fields": []any{"body"}}, `[{"body":"lgtm"}]`}, + {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 1, "fields": []any{}}, `[{"id":9,"state":"APPROVED","body":"lgtm","html_url":"https://github.com/owner/repo/pull/1#pullrequestreview-9","user":{"login":"octocat"},"commit_id":"abc","author_association":"MEMBER"}]`}, {"pull_request_read", map[string]any{"method": "get_comments", "pullNumber": 1}, `[{"id":42,"body":"hello","html_url":"https://github.com/owner/repo/pull/1#issuecomment-42"}]`}, {"pull_request_read", map[string]any{"method": "get_comments", "pullNumber": 2}, `[]`}, {"pull_request_read", map[string]any{"method": "get_check_runs", "pullNumber": 1}, `{"total_count":1,"check_runs":[{"id":5,"name":"build","status":"completed","conclusion":"success"}]}`}, {"pull_request_read", map[string]any{"method": "get_check_runs", "pullNumber": 1, "page": 2}, `{"total_count":0,"check_runs":[]}`}, + // total_count is response metadata and must survive filtering. + {"pull_request_read", map[string]any{"method": "get_check_runs", "pullNumber": 1, "fields": []any{"name", "conclusion"}}, `{"total_count":1,"check_runs":[{"conclusion":"success","name":"build"}]}`}, + {"pull_request_read", map[string]any{"method": "get_check_runs", "pullNumber": 1, "fields": []any{}}, `{"total_count":1,"check_runs":[{"id":5,"name":"build","status":"completed","conclusion":"success"}]}`}, {"create_pull_request", map[string]any{"title": "Subject", "head": "feature", "base": "main", "draft": false}, prRef}, {"update_pull_request", map[string]any{"pullNumber": "1", "title": "Subject", "maintainer_can_modify": false}, prRef}, {"merge_pull_request", map[string]any{"pullNumber": 1, "merge_method": "squash", "expectedHeadSha": "abc"}, `{"sha":"abc","merged":true,"message":"Pull Request successfully merged"}`}, @@ -406,6 +415,9 @@ func TestTypedConsolidatedPullRequestErrors(t *testing.T) { {"pull_request_read", map[string]any{"method": "get", "pullNumber": "1.5"}, "parameter pullNumber is not a valid number: non-integer numeric value: 1.5"}, {"pull_request_read", map[string]any{"method": "get_files", "pullNumber": 1, "page": "x"}, "parameter page is not a valid number"}, {"pull_request_read", map[string]any{"method": "unknown", "pullNumber": 1}, "unknown method: unknown"}, + {"pull_request_read", map[string]any{"method": "get", "pullNumber": 1, "fields": []any{"state"}}, `fields is not supported for pull_request_read method "get"`}, + {"pull_request_read", map[string]any{"method": "get_reviews", "pullNumber": 1, "fields": []any{"name"}}, `field "name" is not supported for pull_request_read method "get_reviews"`}, + {"pull_request_read", map[string]any{"method": "get_check_runs", "pullNumber": 1, "fields": []any{"body"}}, `field "body" is not supported for pull_request_read method "get_check_runs"`}, {"pull_request_read", map[string]any{"method": "get", "pullNumber": 1.5}, "parameter pullNumber is not a valid number: non-integer numeric value: 1.5"}, {"create_pull_request", map[string]any{"title": "Subject", "head": "feature", "base": "main", "draft": "yes"}, "parameter draft is not of type bool, is string"}, {"update_pull_request", map[string]any{"pullNumber": 0, "title": "Subject"}, "missing required parameter: pullNumber"},