diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 2915c53f2..484c43412 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -345,6 +345,54 @@ runtime behavior (such as output formatting) won't appear here. - `repo`: Repository name (string, required) - `sha`: Accepts optional commit SHA. If specified, it will be used instead of ref (string, optional) +- **list_commits** - List commits + - **Required OAuth Scopes**: `repo` + - `author`: Author username or email address to filter commits by (string, optional) + - `fields`: Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'. (string[], optional) + - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `path`: Only commits containing this file path will be returned (string, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `repo`: Repository name (string, required) + - `sha`: Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA. (string, optional) + - `since`: Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) + - `until`: Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) + +- **list_issues** - List issues + - **Required OAuth Scopes**: `repo` + - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) + - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) + - `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional) + - `fields`: Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data. (string[], optional) + - `labels`: Filter by labels (string[], optional) + - `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (string, optional) + - `owner`: Repository owner (string, required) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `repo`: Repository name (string, required) + - `since`: Filter by date (ISO 8601 timestamp) (string, optional) + - `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional) + +- **list_pull_requests** - List pull requests + - **Required OAuth Scopes**: `repo` + - `base`: Filter by base branch (string, optional) + - `direction`: Sort direction (string, optional) + - `fields`: Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data. (string[], optional) + - `head`: Filter by head user/org and branch (string, optional) + - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `repo`: Repository name (string, required) + - `sort`: Sort by (string, optional) + - `state`: Filter by state (string, optional) + +- **list_releases** - List releases + - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data. (string[], optional) + - `owner`: Repository owner (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `repo`: Repository name (string, required) + - **search_code** - Search code - **Required OAuth Scopes**: `repo` - `fields`: Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data. (string[], optional) @@ -354,4 +402,26 @@ runtime behavior (such as output formatting) won't appear here. - `query`: Search query (GitHub code search REST). Implicit AND between terms; supports `OR`, `NOT`, and `"quoted phrase"` for exact match. Qualifiers: `repo:owner/repo`, `org:`, `user:`, `language:`, `path:dir` (prefix match), `filename:exact.ext`, `extension:`, `in:file`, `in:path`, `size:`, `is:archived`, `is:fork`. Max 256 chars. Examples: `WithContext language:go org:github`; `"package main" repo:o/r`; `func extension:go path:cmd repo:o/r`; `NOT TODO language:go repo:o/r`. (string, required) - `sort`: Sort field ('indexed' only) (string, optional) +- **search_issues** - Search issues + - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) + - `order`: Sort order (string, optional) + - `owner`: Optional repository owner. If provided with repo, only issues for this repository are listed. (string, optional) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `query`: Search query using GitHub issues search syntax (string, required) + - `repo`: Optional repository name. If provided with owner, only issues for this repository are listed. (string, optional) + - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) + +- **search_pull_requests** - Search pull requests + - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) + - `order`: Sort order (string, optional) + - `owner`: Optional repository owner. If provided with repo, only pull requests for this repository are listed. (string, optional) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `query`: Search query using GitHub pull request search syntax (string, required) + - `repo`: Optional repository name. If provided with owner, only pull requests for this repository are listed. (string, optional) + - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) + diff --git a/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap new file mode 100644 index 000000000..bc4ffd175 --- /dev/null +++ b/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap @@ -0,0 +1,71 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "List commits" + }, + "description": "Get list of commits of a branch in a GitHub repository. Returns at least 30 results per page by default, but can return more if specified using the perPage parameter (up to 100).", + "inputSchema": { + "properties": { + "author": { + "description": "Author username or email address to filter commits by", + "type": "string" + }, + "fields": { + "description": "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", + "items": { + "enum": [ + "sha", + "html_url", + "commit", + "author", + "committer" + ], + "type": "string" + }, + "type": "array" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "path": { + "description": "Only commits containing this file path will be returned", + "type": "string" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sha": { + "description": "Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA.", + "type": "string" + }, + "since": { + "description": "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + "type": "string" + }, + "until": { + "description": "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + "type": "string" + } + }, + "required": [ + "owner", + "repo" + ], + "type": "object" + }, + "name": "list_commits" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap new file mode 100644 index 000000000..1055fe994 --- /dev/null +++ b/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap @@ -0,0 +1,112 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "List issues" + }, + "description": "List issues in a GitHub repository. For pagination, use the 'endCursor' from the previous response's 'pageInfo' in the 'after' parameter.", + "inputSchema": { + "properties": { + "after": { + "description": "Cursor for pagination. Use the cursor from the previous response.", + "type": "string" + }, + "direction": { + "description": "Order direction. If provided, the 'orderBy' also needs to be provided.", + "enum": [ + "ASC", + "DESC" + ], + "type": "string" + }, + "field_filters": { + "description": "Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date).", + "items": { + "properties": { + "field_name": { + "description": "Name of the custom field (e.g. \"Priority\"). Case-insensitive.", + "type": "string" + }, + "value": { + "description": "Value to filter on. For single-select fields, the option name (e.g. \"P1\"). For dates, YYYY-MM-DD. For numbers, the numeric value as a string. For text, the text value.", + "type": "string" + } + }, + "required": [ + "field_name", + "value" + ], + "type": "object" + }, + "type": "array" + }, + "fields": { + "description": "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "user", + "labels", + "comments", + "created_at", + "updated_at", + "field_values" + ], + "type": "string" + }, + "type": "array" + }, + "labels": { + "description": "Filter by labels", + "items": { + "type": "string" + }, + "type": "array" + }, + "orderBy": { + "description": "Order issues by field. If provided, the 'direction' also needs to be provided.", + "enum": [ + "CREATED_AT", + "UPDATED_AT", + "COMMENTS" + ], + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "since": { + "description": "Filter by date (ISO 8601 timestamp)", + "type": "string" + }, + "state": { + "description": "Filter by state, by default both open and closed issues are returned when not provided", + "enum": [ + "OPEN", + "CLOSED" + ], + "type": "string" + } + }, + "required": [ + "owner", + "repo" + ], + "type": "object" + }, + "name": "list_issues" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap new file mode 100644 index 000000000..d37986d52 --- /dev/null +++ b/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap @@ -0,0 +1,106 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "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.", + "inputSchema": { + "properties": { + "base": { + "description": "Filter by base branch", + "type": "string" + }, + "direction": { + "description": "Sort direction", + "enum": [ + "asc", + "desc" + ], + "type": "string" + }, + "fields": { + "description": "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "draft", + "merged", + "mergeable_state", + "html_url", + "user", + "labels", + "assignees", + "requested_reviewers", + "merged_by", + "head", + "base", + "additions", + "deletions", + "changed_files", + "commits", + "comments", + "created_at", + "updated_at", + "closed_at", + "merged_at", + "milestone" + ], + "type": "string" + }, + "type": "array" + }, + "head": { + "description": "Filter by head user/org and branch", + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "sort": { + "description": "Sort by", + "enum": [ + "created", + "updated", + "popularity", + "long-running" + ], + "type": "string" + }, + "state": { + "description": "Filter by state", + "enum": [ + "open", + "closed", + "all" + ], + "type": "string" + } + }, + "required": [ + "owner", + "repo" + ], + "type": "object" + }, + "name": "list_pull_requests" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap new file mode 100644 index 000000000..4eeef279e --- /dev/null +++ b/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap @@ -0,0 +1,55 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "List releases" + }, + "description": "List releases in a GitHub repository", + "inputSchema": { + "properties": { + "fields": { + "description": "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", + "items": { + "enum": [ + "id", + "tag_name", + "name", + "body", + "html_url", + "published_at", + "prerelease", + "draft", + "author" + ], + "type": "string" + }, + "type": "array" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo" + ], + "type": "object" + }, + "name": "list_releases" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap b/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap new file mode 100644 index 000000000..f705f1472 --- /dev/null +++ b/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap @@ -0,0 +1,98 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "Search issues" + }, + "description": "Search for issues in GitHub repositories using issues search syntax already scoped to is:issue", + "inputSchema": { + "properties": { + "fields": { + "description": "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "state_reason", + "draft", + "locked", + "html_url", + "user", + "author_association", + "labels", + "assignee", + "assignees", + "milestone", + "comments", + "reactions", + "created_at", + "updated_at", + "closed_at", + "closed_by", + "type", + "repository_url", + "pull_request", + "field_values" + ], + "type": "string" + }, + "type": "array" + }, + "order": { + "description": "Sort order", + "enum": [ + "asc", + "desc" + ], + "type": "string" + }, + "owner": { + "description": "Optional repository owner. If provided with repo, only issues for this repository are listed.", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "query": { + "description": "Search query using GitHub issues search syntax", + "type": "string" + }, + "repo": { + "description": "Optional repository name. If provided with owner, only issues for this repository are listed.", + "type": "string" + }, + "sort": { + "description": "Sort field by number of matches of categories, defaults to best match", + "enum": [ + "comments", + "reactions", + "reactions-+1", + "reactions--1", + "reactions-smile", + "reactions-thinking_face", + "reactions-heart", + "reactions-tada", + "interactions", + "created", + "updated" + ], + "type": "string" + } + }, + "required": [ + "query" + ], + "type": "object" + }, + "name": "search_issues" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap b/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap new file mode 100644 index 000000000..847168b47 --- /dev/null +++ b/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap @@ -0,0 +1,96 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "Search pull requests" + }, + "description": "Search for pull requests in GitHub repositories using issues search syntax already scoped to is:pr", + "inputSchema": { + "properties": { + "fields": { + "description": "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "state_reason", + "draft", + "locked", + "html_url", + "user", + "author_association", + "labels", + "assignee", + "assignees", + "milestone", + "comments", + "reactions", + "created_at", + "updated_at", + "closed_at", + "closed_by", + "pull_request", + "repository_url" + ], + "type": "string" + }, + "type": "array" + }, + "order": { + "description": "Sort order", + "enum": [ + "asc", + "desc" + ], + "type": "string" + }, + "owner": { + "description": "Optional repository owner. If provided with repo, only pull requests for this repository are listed.", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "query": { + "description": "Search query using GitHub pull request search syntax", + "type": "string" + }, + "repo": { + "description": "Optional repository name. If provided with owner, only pull requests for this repository are listed.", + "type": "string" + }, + "sort": { + "description": "Sort field by number of matches of categories, defaults to best match", + "enum": [ + "comments", + "reactions", + "reactions-+1", + "reactions--1", + "reactions-smile", + "reactions-thinking_face", + "reactions-heart", + "reactions-tada", + "interactions", + "created", + "updated" + ], + "type": "string" + } + }, + "required": [ + "query" + ], + "type": "object" + }, + "name": "search_pull_requests" +} \ No newline at end of file diff --git a/pkg/github/fields_filtering_test.go b/pkg/github/fields_filtering_test.go new file mode 100644 index 000000000..68af5d17f --- /dev/null +++ b/pkg/github/fields_filtering_test.go @@ -0,0 +1,543 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v87/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- list_commits --------------------------------------------------------- + +func Test_LegacyListCommits_Definition(t *testing.T) { + serverTool := LegacyListCommits(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "list_commits", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +func mockListCommits() []*github.RepositoryCommit { + return []*github.RepositoryCommit{ + { + SHA: github.Ptr("abc123def456"), + HTMLURL: github.Ptr("https://github.com/owner/repo/commit/abc123def456"), + Commit: &github.Commit{ + Message: github.Ptr("First commit with a reasonably long message to add bytes"), + Author: &github.CommitAuthor{ + Name: github.Ptr("Test User"), + Email: github.Ptr("test@example.com"), + }, + }, + Author: &github.User{Login: github.Ptr("testuser")}, + }, + } +} + +func Test_ListCommits_FieldFiltering(t *testing.T) { + serverTool := ListCommits(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepo: mockResponse(t, http.StatusOK, mockListCommits()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "fields": []any{"sha"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + var items []map[string]any + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &items)) + require.Len(t, items, 1) + require.Len(t, items[0], 1) + assert.Contains(t, items[0], "sha") + assert.NotContains(t, textContent.Text, "html_url") + assert.NotContains(t, textContent.Text, "commit") +} + +func Test_ListCommits_FieldsTelemetry(t *testing.T) { + serverTool := ListCommits(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposCommitsByOwnerByRepo: mockResponse(t, http.StatusOK, mockListCommits()), + })) + + assertFieldsTelemetry(t, serverTool, client, "list_commits", + map[string]any{"owner": "owner", "repo": "repo", "fields": []any{"sha"}}, + map[string]any{"owner": "owner", "repo": "repo"}) +} + +// --- list_releases -------------------------------------------------------- + +func Test_LegacyListReleases_Definition(t *testing.T) { + serverTool := LegacyListReleases(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "list_releases", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +func mockListReleases() []*github.RepositoryRelease { + return []*github.RepositoryRelease{ + { + ID: github.Ptr(int64(1)), + TagName: github.Ptr("v1.0.0"), + Name: github.Ptr("First Release"), + Body: github.Ptr("Release notes with a reasonably long body to add bytes"), + HTMLURL: github.Ptr("https://github.com/owner/repo/releases/tag/v1.0.0"), + }, + } +} + +func Test_ListReleases_FieldFiltering(t *testing.T) { + serverTool := ListReleases(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposReleasesByOwnerByRepo: mockResponse(t, http.StatusOK, mockListReleases()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "fields": []any{"tag_name", "name"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + var items []map[string]any + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &items)) + require.Len(t, items, 1) + require.Len(t, items[0], 2) + assert.Contains(t, items[0], "tag_name") + assert.Contains(t, items[0], "name") + assert.NotContains(t, textContent.Text, "body") + assert.NotContains(t, textContent.Text, "html_url") +} + +func Test_ListReleases_FieldsTelemetry(t *testing.T) { + serverTool := ListReleases(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposReleasesByOwnerByRepo: mockResponse(t, http.StatusOK, mockListReleases()), + })) + + assertFieldsTelemetry(t, serverTool, client, "list_releases", + map[string]any{"owner": "owner", "repo": "repo", "fields": []any{"tag_name"}}, + map[string]any{"owner": "owner", "repo": "repo"}) +} + +// --- list_pull_requests --------------------------------------------------- + +func Test_LegacyListPullRequests_Definition(t *testing.T) { + serverTool := LegacyListPullRequests(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "list_pull_requests", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +func mockListPullRequests() []*github.PullRequest { + return []*github.PullRequest{ + { + Number: github.Ptr(42), + Title: github.Ptr("First PR"), + Body: github.Ptr("PR body with a reasonably long description to add bytes"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42"), + User: &github.User{Login: github.Ptr("user1")}, + }, + } +} + +func Test_ListPullRequests_FieldFiltering(t *testing.T) { + serverTool := ListPullRequests(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepo: mockResponse(t, http.StatusOK, mockListPullRequests()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "fields": []any{"number", "title"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + var items []map[string]any + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &items)) + require.Len(t, items, 1) + require.Len(t, items[0], 2) + assert.Contains(t, items[0], "number") + assert.Contains(t, items[0], "title") + assert.NotContains(t, textContent.Text, "html_url") + assert.NotContains(t, textContent.Text, "body") +} + +func Test_ListPullRequests_FieldsTelemetry(t *testing.T) { + serverTool := ListPullRequests(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepo: mockResponse(t, http.StatusOK, mockListPullRequests()), + })) + + assertFieldsTelemetry(t, serverTool, client, "list_pull_requests", + map[string]any{"owner": "owner", "repo": "repo", "fields": []any{"number"}}, + map[string]any{"owner": "owner", "repo": "repo"}) +} + +// --- search_pull_requests ------------------------------------------------- + +func Test_LegacySearchPullRequests_Definition(t *testing.T) { + serverTool := LegacySearchPullRequests(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "search_pull_requests", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +// mockIssueSearchResult returns a single-item issues search result. It is used +// for both search_pull_requests and search_issues since both hit the REST +// issues search endpoint. Issues intentionally omit NodeID so search_issues +// does not attempt the follow-up GraphQL field-values enrichment. +func mockIssueSearchResult() *github.IssuesSearchResult { + return &github.IssuesSearchResult{ + Total: github.Ptr(1), + IncompleteResults: github.Ptr(false), + Issues: []*github.Issue{ + { + Number: github.Ptr(42), + Title: github.Ptr("A result"), + Body: github.Ptr("Body with a reasonably long description to add bytes"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42"), + User: &github.User{Login: github.Ptr("user1")}, + }, + }, + } +} + +func Test_SearchPullRequests_FieldFiltering(t *testing.T) { + serverTool := SearchPullRequests(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetSearchIssues: mockResponse(t, http.StatusOK, mockIssueSearchResult()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "query": "fix", + "fields": []any{"number", "title"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + assertSearchWrapperFiltered(t, textContent.Text) +} + +func Test_SearchPullRequests_FieldsTelemetry(t *testing.T) { + serverTool := SearchPullRequests(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetSearchIssues: mockResponse(t, http.StatusOK, mockIssueSearchResult()), + })) + + assertFieldsTelemetry(t, serverTool, client, "search_pull_requests", + map[string]any{"query": "fix", "fields": []any{"number"}}, + map[string]any{"query": "fix"}) +} + +// --- search_issues -------------------------------------------------------- + +func Test_LegacySearchIssues_Definition(t *testing.T) { + serverTool := LegacySearchIssues(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "search_issues", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +func Test_SearchIssues_FieldFiltering(t *testing.T) { + serverTool := SearchIssues(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetSearchIssues: mockResponse(t, http.StatusOK, mockIssueSearchResult()), + })) + deps := BaseDeps{Client: client} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "query": "bug", + "fields": []any{"number", "title"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + assertSearchWrapperFiltered(t, textContent.Text) +} + +func Test_SearchIssues_FieldsTelemetry(t *testing.T) { + serverTool := SearchIssues(translations.NullTranslationHelper) + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetSearchIssues: mockResponse(t, http.StatusOK, mockIssueSearchResult()), + })) + + assertFieldsTelemetry(t, serverTool, client, "search_issues", + map[string]any{"query": "bug", "fields": []any{"number"}}, + map[string]any{"query": "bug"}) +} + +// --- list_issues (GraphQL) ------------------------------------------------ + +func Test_LegacyListIssues_Definition(t *testing.T) { + serverTool := LegacyListIssues(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) + + assert.Equal(t, "list_issues", tool.Name) + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be *jsonschema.Schema") + assert.NotContains(t, schema.Properties, "fields") +} + +// listIssuesFieldsQuery and listIssuesFieldsVars mirror the exact GraphQL query +// and variables list_issues issues for owner/repo with default parameters (no +// labels, no since). They must stay in sync with the query built in +// getIssueQueryType; see Test_ListIssues for the canonical copies. +const listIssuesFieldsFieldValuesSelection = "issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}" + +const listIssuesFieldsQuery = "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}," + listIssuesFieldsFieldValuesSelection + "},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + +func listIssuesFieldsMockClient() *http.Client { + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "states": []any{"OPEN", "CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": (*string)(nil), + "issueFieldValues": []any{}, + } + response := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issues": map[string]any{ + "nodes": []map[string]any{ + { + "number": 123, + "title": "First Issue", + "body": "This is a reasonably long issue body to add bytes", + "state": "OPEN", + "databaseId": 1001, + "createdAt": "2023-01-01T00:00:00Z", + "updatedAt": "2023-01-01T00:00:00Z", + "author": map[string]any{"login": "user1"}, + "labels": map[string]any{"nodes": []map[string]any{}}, + "comments": map[string]any{"totalCount": 1}, + "issueFieldValues": map[string]any{"nodes": []map[string]any{}}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "", + "endCursor": "", + }, + "totalCount": 1, + }, + "isPrivate": false, + }, + }) + matcher := githubv4mock.NewQueryMatcher(listIssuesFieldsQuery, vars, response) + return githubv4mock.NewMockedHTTPClient(matcher) +} + +func Test_ListIssues_FieldFiltering(t *testing.T) { + serverTool := ListIssues(translations.NullTranslationHelper) + deps := BaseDeps{GQLClient: githubv4.NewClient(listIssuesFieldsMockClient())} + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "fields": []any{"number", "title"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + + // The wrapper metadata is preserved while each issue is reduced to the + // requested fields only. + var returned struct { + Issues []map[string]any `json:"issues"` + TotalCount int `json:"totalCount"` + PageInfo map[string]any `json:"pageInfo"` + } + require.NoError(t, json.Unmarshal([]byte(textContent.Text), &returned)) + assert.Equal(t, 1, returned.TotalCount) + require.NotNil(t, returned.PageInfo) + require.Len(t, returned.Issues, 1) + require.Len(t, returned.Issues[0], 2) + assert.Contains(t, returned.Issues[0], "number") + assert.Contains(t, returned.Issues[0], "title") + assert.NotContains(t, textContent.Text, "\"body\"") +} + +func Test_ListIssues_FieldsTelemetry(t *testing.T) { + serverTool := ListIssues(translations.NullTranslationHelper) + + t.Run("filtered call records savings", func(t *testing.T) { + deps, rec := depsWithRecordingMetrics(t, BaseDeps{GQLClient: githubv4.NewClient(listIssuesFieldsMockClient())}) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "fields": []any{"number"}, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + assertFilteredCounters(t, rec, "list_issues") + }) + + t.Run("unfiltered call records adoption only", func(t *testing.T) { + deps, rec := depsWithRecordingMetrics(t, BaseDeps{GQLClient: githubv4.NewClient(listIssuesFieldsMockClient())}) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + call, ok := rec.increment(metricFieldsToolCall) + require.True(t, ok) + assert.Equal(t, "false", call.tags["filtered"]) + _, ok = rec.counter(metricFieldsBytesFull) + assert.False(t, ok, "no byte counters when not filtered") + }) +} + +// --- shared assertion helpers --------------------------------------------- + +// assertSearchWrapperFiltered asserts that a filtered search response preserves +// the total_count / incomplete_results wrapper while reducing each item to the +// requested number/title fields only. +func assertSearchWrapperFiltered(t *testing.T, text string) { + t.Helper() + var returned struct { + TotalCount int `json:"total_count"` + IncompleteResults bool `json:"incomplete_results"` + Items []map[string]any `json:"items"` + } + require.NoError(t, json.Unmarshal([]byte(text), &returned)) + assert.Equal(t, 1, returned.TotalCount) + require.Len(t, returned.Items, 1) + require.Len(t, returned.Items[0], 2) + assert.Contains(t, returned.Items[0], "number") + assert.Contains(t, returned.Items[0], "title") + assert.NotContains(t, text, "html_url") + assert.NotContains(t, text, "\"body\"") +} + +// assertFilteredCounters asserts the full set of counters emitted for a filtered +// call: an increment tagged filtered=true plus positive byte counters where +// full > sent and saved == full - sent. +func assertFilteredCounters(t *testing.T, rec *recordingMetrics, tool string) { + t.Helper() + call, ok := rec.increment(metricFieldsToolCall) + require.True(t, ok) + assert.Equal(t, tool, call.tags["tool"]) + assert.Equal(t, "true", call.tags["filtered"]) + + full, ok := rec.counter(metricFieldsBytesFull) + require.True(t, ok) + sent, ok := rec.counter(metricFieldsBytesSent) + require.True(t, ok) + saved, ok := rec.counter(metricFieldsBytesSaved) + require.True(t, ok) + assert.Greater(t, full.value, sent.value, "filtering should remove bytes") + assert.Equal(t, full.value-sent.value, saved.value) +} + +// assertFieldsTelemetry runs a filtered and an unfiltered call against the given +// tool and asserts the expected adoption and savings telemetry for each. +func assertFieldsTelemetry(t *testing.T, serverTool inventory.ServerTool, client *github.Client, tool string, filteredArgs, unfilteredArgs map[string]any) { + t.Helper() + + t.Run("filtered call records savings", func(t *testing.T) { + deps, rec := depsWithRecordingMetrics(t, BaseDeps{Client: client}) + handler := serverTool.Handler(deps) + + request := createMCPRequest(filteredArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + assertFilteredCounters(t, rec, tool) + }) + + t.Run("unfiltered call records adoption only", func(t *testing.T) { + deps, rec := depsWithRecordingMetrics(t, BaseDeps{Client: client}) + handler := serverTool.Handler(deps) + + request := createMCPRequest(unfilteredArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + call, ok := rec.increment(metricFieldsToolCall) + require.True(t, ok) + assert.Equal(t, "false", call.tags["filtered"]) + _, ok = rec.counter(metricFieldsBytesFull) + assert.False(t, ok, "no byte counters when not filtered") + }) +} diff --git a/pkg/github/fields_param_gating_test.go b/pkg/github/fields_param_gating_test.go index 09b176914..1e61d4eb8 100644 --- a/pkg/github/fields_param_gating_test.go +++ b/pkg/github/fields_param_gating_test.go @@ -11,14 +11,23 @@ import ( ) // Test_FieldsParamVariants_MutuallyExclusive guards the dual-variant -// registration for the fields_param feature flag. The flag-enabled tools -// (search_code, get_file_contents) and their Legacy* counterparts share a tool -// name, so exactly one of each pair must survive inventory filtering for any -// flag state. If both ever leaked, a client could be offered two tools with the -// same name. This asserts that each gated tool is present exactly once, -// advertising the `fields` parameter only when fields_param is enabled. +// registration for the fields_param feature flag. The flag-enabled tools and +// their Legacy* counterparts share a tool name, so exactly one of each pair must +// survive inventory filtering for any flag state. If both ever leaked, a client +// could be offered two tools with the same name. This asserts that each gated +// tool is present exactly once, advertising the `fields` parameter only when +// fields_param is enabled. func Test_FieldsParamVariants_MutuallyExclusive(t *testing.T) { - gatedTools := []string{"search_code", "get_file_contents"} + gatedTools := []string{ + "search_code", + "get_file_contents", + "list_issues", + "list_releases", + "list_pull_requests", + "search_issues", + "search_pull_requests", + "list_commits", + } for _, tc := range []struct { name string diff --git a/pkg/github/fields_telemetry.go b/pkg/github/fields_telemetry.go index 324f463be..47ce430a1 100644 --- a/pkg/github/fields_telemetry.go +++ b/pkg/github/fields_telemetry.go @@ -2,6 +2,7 @@ package github import ( "context" + "encoding/json" "strconv" ) @@ -52,3 +53,19 @@ func recordFieldsUsage(ctx context.Context, deps ToolDependencies, tool string, m.Counter(metricFieldsBytesSent, toolTag, int64(sentBytes)) m.Counter(metricFieldsBytesSaved, toolTag, int64(saved)) } + +// recordFieldsUsageFor emits fields telemetry for a tool whose response is a +// list of items (optionally wrapped in a metadata envelope). sentBytes is the +// size of the payload actually returned. When the response was filtered, the +// unfiltered size is computed by marshalling full so the realized savings can be +// measured; full should be the complete, unfiltered payload. It centralizes the +// full-size computation shared by every fields-enabled tool. +func recordFieldsUsageFor(ctx context.Context, deps ToolDependencies, tool string, full any, filtered bool, sentBytes int) { + fullBytes := sentBytes + if filtered { + if data, err := json.Marshal(full); err == nil { + fullBytes = len(data) + } + } + recordFieldsUsage(ctx, deps, tool, filtered, fullBytes, sentBytes) +} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 62db8d48d..87c6df940 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1527,7 +1527,34 @@ func ReprioritizeSubIssue(ctx context.Context, client *github.Client, owner stri } // SearchIssues creates a tool to search for issues. +// SearchIssues creates a tool to search for issues. It is the +// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` +// parameter and filters each result to the requested subset. Both this and +// LegacySearchIssues register under the tool name "search_issues"; exactly one is +// active for any given request thanks to mutually exclusive FeatureFlagEnable / +// FeatureFlagDisable annotations. func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + st := searchIssuesTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacySearchIssues is the FeatureFlagFieldsParam-disabled variant of +// search_issues. It exposes the original schema (no `fields` parameter) and never +// filters results, so it acts as the kill switch when the flag is off. It owns +// the canonical search_issues.snap; the flag-enabled variant owns +// search_issues_ff_.snap. Delete this function when the flag is removed. +func LegacySearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + st := searchIssuesTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// searchIssuesTool builds the search_issues tool. When includeFields is true the +// tool advertises the optional `fields` parameter, filters each result to the +// requested subset, and emits fields telemetry. When false it is the original +// tool with no fields parameter and no filtering. +func searchIssuesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1568,6 +1595,12 @@ func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { }, Required: []string{"query"}, } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + searchIssuesItemFieldEnum, + ) + } WithPagination(schema) return NewTool( @@ -1583,7 +1616,15 @@ func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - result, err := searchIssuesHandler(ctx, deps, args, ifcSearchPostProcessOption(ctx, deps)) + options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} + if includeFields { + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + options = append(options, withFieldsFiltering(deps, "search_issues", fields)) + } + result, err := searchIssuesHandler(ctx, deps, args, options...) return result, nil, err }) } @@ -1812,16 +1853,36 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st Items: items, } - r, err := json.Marshal(response) + cfg := searchConfig{} + for _, opt := range options { + opt(&cfg) + } + + filtered := false + var payload any = response + if len(cfg.fields) > 0 { + filteredItems, err := filterEachField(response.Items, cfg.fields) + if err != nil { + return utils.NewToolResultErrorFromErr(errorPrefix+": failed to filter results", err), nil + } + payload = map[string]any{ + "total_count": response.Total, + "incomplete_results": response.IncompleteResults, + "items": filteredItems, + } + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { return utils.NewToolResultErrorFromErr(errorPrefix+": failed to marshal response", err), nil } - callResult := utils.NewToolResultText(string(r)) - cfg := searchConfig{} - for _, opt := range options { - opt(&cfg) + if cfg.fieldsTool != "" { + recordFieldsUsageFor(ctx, cfg.fieldsDeps, cfg.fieldsTool, response, filtered, len(r)) } + + callResult := utils.NewToolResultText(string(r)) if cfg.postProcess != nil { cfg.postProcess(ctx, result, callResult) } @@ -2433,7 +2494,34 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 // ListIssues creates a tool to list and filter repository issues. It exposes the // Issues 2.0 field_filters input plus field_values output enrichment. +// ListIssues creates a tool to list issues in a GitHub repository. It is the +// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` +// parameter and filters each issue to the requested subset. Both this and +// LegacyListIssues register under the tool name "list_issues"; exactly one is +// active for any given request thanks to mutually exclusive FeatureFlagEnable / +// FeatureFlagDisable annotations. func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listIssuesTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacyListIssues is the FeatureFlagFieldsParam-disabled variant of list_issues. +// It exposes the original schema (no `fields` parameter) and never filters +// results, so it acts as the kill switch when the flag is off. It owns the +// canonical list_issues.snap; the flag-enabled variant owns +// list_issues_ff_.snap. Delete this function when the flag is removed. +func LegacyListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listIssuesTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// listIssuesTool builds the list_issues tool. When includeFields is true the +// tool advertises the optional `fields` parameter, filters each issue to the +// requested subset, and emits fields telemetry. When false it is the original +// tool with no fields parameter and no filtering. +func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -2492,6 +2580,12 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { }, Required: []string{"owner", "repo"}, } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", + listIssuesItemFieldEnum, + ) + } WithCursorPagination(schema) st := NewTool( @@ -2516,6 +2610,14 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(err.Error()), nil, nil } + var fields []string + if includeFields { + fields, err = OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + } + // Set optional parameters if provided state, err := OptionalParam[string](args, "state") if err != nil { @@ -2685,7 +2787,31 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { isPrivate = queryResult.GetIsPrivate() } - result := MarshalledTextResult(resp) + filtered := false + var payload any = resp + if includeFields && len(fields) > 0 { + filteredIssues, err := filterEachField(resp.Issues, fields) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to filter issues", err), nil, nil + } + payload = map[string]any{ + "issues": filteredIssues, + "totalCount": resp.TotalCount, + "pageInfo": resp.PageInfo, + } + filtered = true + } + + r, err := json.Marshal(payload) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil + } + + if includeFields { + recordFieldsUsageFor(ctx, deps, "list_issues", resp, filtered, len(r)) + } + + result := utils.NewToolResultText(string(r)) result = attachStaticIFCLabel(ctx, deps, result, ifc.LabelListIssues(isPrivate)) return result, nil, nil }) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3be4fe1b5..2b71d821a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -564,7 +564,11 @@ func Test_SearchIssues(t *testing.T) { // Verify tool definition once serverTool := SearchIssues(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + // SearchIssues is the FeatureFlagFieldsParam-enabled variant; it owns the + // _ff_ snapshot. The canonical search_issues.snap is owned by + // LegacySearchIssues (see Test_LegacySearchIssues_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) assert.Equal(t, "search_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -575,6 +579,7 @@ func Test_SearchIssues(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "order") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "perPage") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "page") + assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "fields") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"query"}) // Setup mock search results @@ -1644,8 +1649,11 @@ func Test_ListIssues(t *testing.T) { // Verify tool definition serverTool := ListIssues(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Empty(t, serverTool.FeatureFlagEnable) + // ListIssues is the FeatureFlagFieldsParam-enabled variant; it owns the + // _ff_ snapshot. The canonical list_issues.snap is owned by + // LegacyListIssues (see Test_LegacyListIssues_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) assert.Equal(t, "list_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -1658,6 +1666,7 @@ func Test_ListIssues(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "since") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "after") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "perPage") + assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "fields") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"owner", "repo"}) // Mock issues data diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index 467fcda2c..f2c0e7b3f 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -10,6 +10,7 @@ import ( "time" "github.com/google/go-github/v87/github" + "github.com/google/jsonschema-go/jsonschema" "github.com/github/github-mcp-server/pkg/sanitize" ) @@ -26,6 +27,71 @@ var codeSearchItemFieldEnum = []any{"name", "path", "sha", "repository", "text_m // the requested path is a directory; ignored for single files. var fileContentFieldEnum = []any{"type", "name", "path", "size", "sha", "url", "git_url", "html_url", "download_url"} +// listIssuesItemFieldEnum lists the selectable fields for list_issues result +// items, matching the JSON field names MinimalIssue actually populates via the +// list_issues GraphQL fragment (fragmentToMinimalIssue). Fields that only the +// REST conversion sets (for example html_url, reactions, issue_field_values) are +// never emitted here and are intentionally omitted. The body and field_values +// fields are the heaviest, so omitting them is the main lever for shrinking large +// result sets. +var listIssuesItemFieldEnum = []any{ + "number", "title", "body", "state", "user", "labels", + "comments", "created_at", "updated_at", "field_values", +} + +// listPullRequestsItemFieldEnum lists the selectable fields for +// list_pull_requests result items, matching the JSON field names of +// MinimalPullRequest. The body field is the heaviest, so omitting it is the main +// lever for shrinking large result sets. +var listPullRequestsItemFieldEnum = []any{ + "number", "title", "body", "state", "draft", "merged", "mergeable_state", + "html_url", "user", "labels", "assignees", "requested_reviewers", "merged_by", + "head", "base", "additions", "deletions", "changed_files", "commits", + "comments", "created_at", "updated_at", "closed_at", "merged_at", "milestone", +} + +// listCommitsItemFieldEnum lists the selectable fields for list_commits result +// items, matching the JSON field names MinimalCommit populates for list_commits. +// list_commits requests commits without per-file detail (commitDetailNone), so +// the stats and files fields are never emitted and are intentionally omitted +// here. The commit field (message plus author/committer metadata) is the +// heaviest, so omitting it is the main lever for shrinking large result sets. +var listCommitsItemFieldEnum = []any{ + "sha", "html_url", "commit", "author", "committer", +} + +// listReleasesItemFieldEnum lists the selectable fields for list_releases result +// items, matching the JSON field names of MinimalRelease. The body field is the +// heaviest, so omitting it is the main lever for shrinking large result sets. +var listReleasesItemFieldEnum = []any{ + "id", "tag_name", "name", "body", "html_url", "published_at", + "prerelease", "draft", "author", +} + +// searchIssuesItemFieldEnum lists the selectable fields for search_issues result +// items. Items are full github.Issue objects enriched with normalized +// field_values, so this is a curated subset of the most useful JSON field names. +// The body, reactions, and labels fields are the heaviest, so omitting them is +// the main lever for shrinking large result sets. +var searchIssuesItemFieldEnum = []any{ + "number", "title", "body", "state", "state_reason", "draft", "locked", + "html_url", "user", "author_association", "labels", "assignee", "assignees", + "milestone", "comments", "reactions", "created_at", "updated_at", "closed_at", + "closed_by", "type", "repository_url", "pull_request", "field_values", +} + +// searchPullRequestsItemFieldEnum lists the selectable fields for +// search_pull_requests result items. Issue search returns pull requests as +// github.Issue objects, so this is a curated subset of those JSON field names. +// The body, reactions, and labels fields are the heaviest, so omitting them is +// the main lever for shrinking large result sets. +var searchPullRequestsItemFieldEnum = []any{ + "number", "title", "body", "state", "state_reason", "draft", "locked", + "html_url", "user", "author_association", "labels", "assignee", "assignees", + "milestone", "comments", "reactions", "created_at", "updated_at", "closed_at", + "closed_by", "pull_request", "repository_url", +} + // 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. @@ -65,6 +131,20 @@ func filterEachField[T any](items []T, fields []string) ([]map[string]any, error return filtered, nil } +// fieldsSchemaProperty builds the optional `fields` array parameter shared by +// every fields-enabled tool: an array of strings constrained to the given enum +// of selectable field names, with a per-tool description. +func fieldsSchemaProperty(description string, enum []any) *jsonschema.Schema { + return &jsonschema.Schema{ + Type: "array", + Description: description, + Items: &jsonschema.Schema{ + Type: "string", + Enum: enum, + }, + } +} + // MinimalUser is the output type for user and organization search results. type MinimalUser struct { Login string `json:"login"` diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 9fc845598..043fbc0f4 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1295,7 +1295,35 @@ func AddReplyToPullRequestComment(t translations.TranslationHelperFunc) inventor } // ListPullRequests creates a tool to list and filter repository pull requests. +// ListPullRequests creates a tool to list pull requests in a GitHub repository. +// It is the FeatureFlagFieldsParam-enabled variant: it advertises the optional +// `fields` parameter and filters each pull request to the requested subset. Both +// this and LegacyListPullRequests register under the tool name +// "list_pull_requests"; exactly one is active for any given request thanks to +// mutually exclusive FeatureFlagEnable / FeatureFlagDisable annotations. func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listPullRequestsTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacyListPullRequests is the FeatureFlagFieldsParam-disabled variant of +// list_pull_requests. It exposes the original schema (no `fields` parameter) and +// never filters results, so it acts as the kill switch when the flag is off. It +// owns the canonical list_pull_requests.snap; the flag-enabled variant owns +// list_pull_requests_ff_.snap. Delete this function when the flag is +// removed. +func LegacyListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listPullRequestsTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// listPullRequestsTool builds the list_pull_requests tool. When includeFields is +// true the tool advertises the optional `fields` parameter, filters each pull +// request to the requested subset, and emits fields telemetry. When false it is +// the original tool with no fields parameter and no filtering. +func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1333,6 +1361,12 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool }, Required: []string{"owner", "repo"}, } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", + listPullRequestsItemFieldEnum, + ) + } WithPagination(schema) return NewTool( @@ -1376,6 +1410,13 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + var fields []string + if includeFields { + fields, err = OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + } pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -1435,11 +1476,26 @@ func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool } } - r, err := json.Marshal(minimalPRs) + filtered := false + var payload any = minimalPRs + if includeFields && len(fields) > 0 { + filteredPRs, err := filterEachField(minimalPRs, fields) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to filter pull requests", err), nil, nil + } + payload = filteredPRs + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil } + if includeFields { + recordFieldsUsageFor(ctx, deps, "list_pull_requests", minimalPRs, filtered, len(r)) + } + result := utils.NewToolResultText(string(r)) // Pull request titles/bodies are user-authored (untrusted); // confidentiality follows repo visibility. @@ -1558,7 +1614,35 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool } // SearchPullRequests creates a tool to search for pull requests. +// SearchPullRequests creates a tool to search for pull requests. It is the +// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` +// parameter and filters each result to the requested subset. Both this and +// LegacySearchPullRequests register under the tool name "search_pull_requests"; +// exactly one is active for any given request thanks to mutually exclusive +// FeatureFlagEnable / FeatureFlagDisable annotations. func SearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { + st := searchPullRequestsTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacySearchPullRequests is the FeatureFlagFieldsParam-disabled variant of +// search_pull_requests. It exposes the original schema (no `fields` parameter) +// and never filters results, so it acts as the kill switch when the flag is off. +// It owns the canonical search_pull_requests.snap; the flag-enabled variant owns +// search_pull_requests_ff_.snap. Delete this function when the flag is +// removed. +func LegacySearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { + st := searchPullRequestsTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// searchPullRequestsTool builds the search_pull_requests tool. When +// includeFields is true the tool advertises the optional `fields` parameter, +// filters each result to the requested subset, and emits fields telemetry. When +// false it is the original tool with no fields parameter and no filtering. +func searchPullRequestsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1599,6 +1683,12 @@ func SearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTo }, Required: []string{"query"}, } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + searchPullRequestsItemFieldEnum, + ) + } WithPagination(schema) return NewTool( @@ -1614,7 +1704,15 @@ func SearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTo }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - result, err := searchHandler(ctx, deps.GetClient, args, "pr", "failed to search pull requests", ifcSearchPostProcessOption(ctx, deps)) + options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} + if includeFields { + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + options = append(options, withFieldsFiltering(deps, "search_pull_requests", fields)) + } + result, err := searchHandler(ctx, deps.GetClient, args, "pr", "failed to search pull requests", options...) return result, nil, err }) } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index bcae87c94..18e769a9a 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -574,7 +574,11 @@ func Test_ListPullRequests(t *testing.T) { // Verify tool definition once serverTool := ListPullRequests(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + // ListPullRequests is the FeatureFlagFieldsParam-enabled variant; it owns + // the _ff_ snapshot. The canonical list_pull_requests.snap is owned by + // LegacyListPullRequests (see Test_LegacyListPullRequests_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) assert.Equal(t, "list_pull_requests", tool.Name) assert.NotEmpty(t, tool.Description) @@ -588,6 +592,7 @@ func Test_ListPullRequests(t *testing.T) { assert.Contains(t, schema.Properties, "direction") assert.Contains(t, schema.Properties, "perPage") assert.Contains(t, schema.Properties, "page") + assert.Contains(t, schema.Properties, "fields") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo"}) // Setup mock PRs for success case @@ -820,7 +825,11 @@ func Test_MergePullRequest(t *testing.T) { func Test_SearchPullRequests(t *testing.T) { serverTool := SearchPullRequests(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + // SearchPullRequests is the FeatureFlagFieldsParam-enabled variant; it owns + // the _ff_ snapshot. The canonical search_pull_requests.snap is owned + // by LegacySearchPullRequests (see Test_LegacySearchPullRequests_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) assert.Equal(t, "search_pull_requests", tool.Name) assert.NotEmpty(t, tool.Description) @@ -832,6 +841,7 @@ func Test_SearchPullRequests(t *testing.T) { assert.Contains(t, schema.Properties, "order") assert.Contains(t, schema.Properties, "perPage") assert.Contains(t, schema.Properties, "page") + assert.Contains(t, schema.Properties, "fields") assert.ElementsMatch(t, schema.Required, []string{"query"}) mockSearchResult := &github.IssuesSearchResult{ diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 2e0430ac0..0c0b1029e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -133,7 +133,76 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { } // ListCommits creates a tool to get commits of a branch in a repository. +// ListCommits creates a tool to get the list of commits of a branch in a GitHub +// repository. It is the FeatureFlagFieldsParam-enabled variant: it advertises +// the optional `fields` parameter and filters each commit to the requested +// subset. Both this and LegacyListCommits register under the tool name +// "list_commits"; exactly one is active for any given request thanks to mutually +// exclusive FeatureFlagEnable / FeatureFlagDisable annotations. func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listCommitsTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacyListCommits is the FeatureFlagFieldsParam-disabled variant of +// list_commits. It exposes the original schema (no `fields` parameter) and never +// filters results, so it acts as the kill switch when the flag is off. It owns +// the canonical list_commits.snap; the flag-enabled variant owns +// list_commits_ff_.snap. Delete this function when the flag is removed. +func LegacyListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listCommitsTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// listCommitsTool builds the list_commits tool. When includeFields is true the +// tool advertises the optional `fields` parameter, filters each commit to the +// requested subset, and emits fields telemetry. When false it is the original +// tool with no fields parameter and no filtering. +func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { + schema := &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "sha": { + Type: "string", + Description: "Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA.", + }, + "author": { + Type: "string", + Description: "Author username or email address to filter commits by", + }, + "path": { + Type: "string", + Description: "Only commits containing this file path will be returned", + }, + "since": { + Type: "string", + Description: "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + }, + "until": { + Type: "string", + Description: "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", + }, + }, + Required: []string{"owner", "repo"}, + } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", + listCommitsItemFieldEnum, + ) + } + WithPagination(schema) + return NewTool( ToolsetMetadataRepos, mcp.Tool{ @@ -143,40 +212,7 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { Title: t("TOOL_LIST_COMMITS_USER_TITLE", "List commits"), ReadOnlyHint: true, }, - InputSchema: WithPagination(&jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "owner": { - Type: "string", - Description: "Repository owner", - }, - "repo": { - Type: "string", - Description: "Repository name", - }, - "sha": { - Type: "string", - Description: "Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA.", - }, - "author": { - Type: "string", - Description: "Author username or email address to filter commits by", - }, - "path": { - Type: "string", - Description: "Only commits containing this file path will be returned", - }, - "since": { - Type: "string", - Description: "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", - }, - "until": { - Type: "string", - Description: "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", - }, - }, - Required: []string{"owner", "repo"}, - }), + InputSchema: schema, }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { @@ -200,6 +236,13 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + var fields []string + if includeFields { + fields, err = OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + } sinceStr, err := OptionalParam[string](args, "since") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -269,11 +312,26 @@ func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { minimalCommits[i] = convertToMinimalCommit(commit, commitDetailNone) } - r, err := json.Marshal(minimalCommits) + filtered := false + var payload any = minimalCommits + if includeFields && len(fields) > 0 { + filteredCommits, err := filterEachField(minimalCommits, fields) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to filter commits", err), nil, nil + } + payload = filteredCommits + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) } + if includeFields { + recordFieldsUsageFor(ctx, deps, "list_commits", minimalCommits, filtered, len(r)) + } + result := utils.NewToolResultText(string(r)) // Commit content is reachable from the repo's history; integrity // follows the same public-untrusted / private-trusted rule as file @@ -750,14 +808,10 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo Required: []string{"owner", "repo"}, } if includeFields { - schema.Properties["fields"] = &jsonschema.Schema{ - Type: "array", - Description: "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", - Items: &jsonschema.Schema{ - Type: "string", - Enum: fileContentFieldEnum, - }, - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", + fileContentFieldEnum, + ) } return NewTool( @@ -957,16 +1011,8 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo // recordDirContentsFieldsUsage emits fields telemetry for a get_file_contents // directory listing. sentBytes is the size of the payload actually returned. -// When the listing was filtered, the unfiltered size is computed from the full -// directory content so the realized savings can be measured. func recordDirContentsFieldsUsage(ctx context.Context, deps ToolDependencies, full []*github.RepositoryContent, filtered bool, sentBytes int) { - fullBytes := sentBytes - if filtered { - if data, err := json.Marshal(full); err == nil { - fullBytes = len(data) - } - } - recordFieldsUsage(ctx, deps, "get_file_contents", filtered, fullBytes, sentBytes) + recordFieldsUsageFor(ctx, deps, "get_file_contents", full, filtered, sentBytes) } // ForkRepository creates a tool to fork a repository. @@ -1806,7 +1852,56 @@ func GetTag(t translations.TranslationHelperFunc) inventory.ServerTool { } // ListReleases creates a tool to list releases in a GitHub repository. +// ListReleases creates a tool to list releases in a GitHub repository. It is the +// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` +// parameter and filters each release to the requested subset. Both this and +// LegacyListReleases register under the tool name "list_releases"; exactly one is +// active for any given request thanks to mutually exclusive FeatureFlagEnable / +// FeatureFlagDisable annotations. func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listReleasesTool(t, true) + st.FeatureFlagEnable = FeatureFlagFieldsParam + return st +} + +// LegacyListReleases is the FeatureFlagFieldsParam-disabled variant of +// list_releases. It exposes the original schema (no `fields` parameter) and never +// filters results, so it acts as the kill switch when the flag is off. It owns +// the canonical list_releases.snap; the flag-enabled variant owns +// list_releases_ff_.snap. Delete this function when the flag is removed. +func LegacyListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { + st := listReleasesTool(t, false) + st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} + return st +} + +// listReleasesTool builds the list_releases tool. When includeFields is true the +// tool advertises the optional `fields` parameter, filters each release to the +// requested subset, and emits fields telemetry. When false it is the original +// tool with no fields parameter and no filtering. +func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { + schema := &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + }, + Required: []string{"owner", "repo"}, + } + if includeFields { + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", + listReleasesItemFieldEnum, + ) + } + WithPagination(schema) + return NewTool( ToolsetMetadataRepos, mcp.Tool{ @@ -1816,20 +1911,7 @@ func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { Title: t("TOOL_LIST_RELEASES_USER_TITLE", "List releases"), ReadOnlyHint: true, }, - InputSchema: WithPagination(&jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "owner": { - Type: "string", - Description: "Repository owner", - }, - "repo": { - Type: "string", - Description: "Repository name", - }, - }, - Required: []string{"owner", "repo"}, - }), + InputSchema: schema, }, []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { @@ -1841,6 +1923,13 @@ func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + var fields []string + if includeFields { + fields, err = OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + } pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -1877,11 +1966,26 @@ func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { } } - r, err := json.Marshal(minimalReleases) + filtered := false + var payload any = minimalReleases + if includeFields && len(fields) > 0 { + filteredReleases, err := filterEachField(minimalReleases, fields) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to filter releases", err), nil, nil + } + payload = filteredReleases + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { return nil, nil, fmt.Errorf("failed to marshal response: %w", err) } + if includeFields { + recordFieldsUsageFor(ctx, deps, "list_releases", minimalReleases, filtered, len(r)) + } + result := utils.NewToolResultText(string(r)) // Releases are published by collaborators with push access, so // integrity is trusted. Confidentiality follows repo visibility, diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 136aa9369..dd6612e23 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -1405,7 +1405,11 @@ func Test_ListCommits(t *testing.T) { // Verify tool definition once serverTool := ListCommits(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + // ListCommits is the FeatureFlagFieldsParam-enabled variant; it owns the + // _ff_ snapshot. The canonical list_commits.snap is owned by + // LegacyListCommits (see Test_LegacyListCommits_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") @@ -1421,6 +1425,7 @@ func Test_ListCommits(t *testing.T) { assert.Contains(t, schema.Properties, "until") assert.Contains(t, schema.Properties, "page") assert.Contains(t, schema.Properties, "perPage") + assert.Contains(t, schema.Properties, "fields") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo"}) // Setup mock commits for success case @@ -3642,7 +3647,11 @@ func Test_GetTag(t *testing.T) { func Test_ListReleases(t *testing.T) { serverTool := ListReleases(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + // ListReleases is the FeatureFlagFieldsParam-enabled variant; it owns the + // _ff_ snapshot. The canonical list_releases.snap is owned by + // LegacyListReleases (see Test_LegacyListReleases_Definition). + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) + require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") @@ -3651,6 +3660,7 @@ func Test_ListReleases(t *testing.T) { assert.NotEmpty(t, tool.Description) assert.Contains(t, schema.Properties, "owner") assert.Contains(t, schema.Properties, "repo") + assert.Contains(t, schema.Properties, "fields") assert.ElementsMatch(t, schema.Required, []string{"owner", "repo"}) mockReleases := []*github.RepositoryRelease{ diff --git a/pkg/github/search.go b/pkg/github/search.go index e84f26187..8472b047c 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -239,14 +239,10 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in Required: []string{"query"}, } if includeFields { - schema.Properties["fields"] = &jsonschema.Schema{ - Type: "array", - Description: "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", - Items: &jsonschema.Schema{ - Type: "string", - Enum: codeSearchItemFieldEnum, - }, - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", + codeSearchItemFieldEnum, + ) } WithPagination(schema) @@ -381,17 +377,9 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in } // recordSearchCodeFieldsUsage emits fields telemetry for a search_code call. -// sentBytes is the size of the payload actually returned. When the response was -// filtered, the unfiltered size is computed from the full minimal result so the -// realized savings can be measured. +// sentBytes is the size of the payload actually returned. func recordSearchCodeFieldsUsage(ctx context.Context, deps ToolDependencies, full *MinimalCodeSearchResult, filtered bool, sentBytes int) { - fullBytes := sentBytes - if filtered { - if data, err := json.Marshal(full); err == nil { - fullBytes = len(data) - } - } - recordFieldsUsage(ctx, deps, "search_code", filtered, fullBytes, sentBytes) + recordFieldsUsageFor(ctx, deps, "search_code", full, filtered, sentBytes) } func userOrOrgHandler(ctx context.Context, accountType string, deps ToolDependencies, args map[string]any) (*mcp.CallToolResult, any, error) { diff --git a/pkg/github/search_utils.go b/pkg/github/search_utils.go index 54213a240..a1fbbc256 100644 --- a/pkg/github/search_utils.go +++ b/pkg/github/search_utils.go @@ -45,6 +45,12 @@ type searchPostProcessFn func(ctx context.Context, result *github.IssuesSearchRe type searchConfig struct { postProcess searchPostProcessFn + // fields, when non-empty, restricts each result item to the requested + // subset of fields. fieldsTool and fieldsDeps identify the calling tool and + // its dependencies so fields telemetry can be recorded. + fields []string + fieldsTool string + fieldsDeps ToolDependencies } type searchOption func(*searchConfig) @@ -55,6 +61,19 @@ func withSearchPostProcess(fn searchPostProcessFn) searchOption { return func(c *searchConfig) { c.postProcess = fn } } +// withFieldsFiltering enables the optional `fields` response filtering for a +// search tool. When fields is non-empty, each result item is reduced to the +// requested subset while the total_count / incomplete_results wrapper is +// preserved. tool and deps identify the caller so fields telemetry (adoption and +// realized savings) can be recorded. +func withFieldsFiltering(deps ToolDependencies, tool string, fields []string) searchOption { + return func(c *searchConfig) { + c.fieldsDeps = deps + c.fieldsTool = tool + c.fields = fields + } +} + // prepareSearchArgs resolves the search query string and REST search options from the tool args, // applying the standard is: / repo:/ munging shared by search_issues and // search_pull_requests. @@ -147,11 +166,30 @@ func searchHandler( return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, errorPrefix, resp, body), nil } - r, err := json.Marshal(result) + filtered := false + var payload any = result + if len(cfg.fields) > 0 { + filteredItems, err := filterEachField(result.Issues, cfg.fields) + if err != nil { + return utils.NewToolResultErrorFromErr(errorPrefix+": failed to filter results", err), nil + } + payload = map[string]any{ + "total_count": result.Total, + "incomplete_results": result.IncompleteResults, + "items": filteredItems, + } + filtered = true + } + + r, err := json.Marshal(payload) if err != nil { return utils.NewToolResultErrorFromErr(errorPrefix+": failed to marshal response", err), nil } + if cfg.fieldsTool != "" { + recordFieldsUsageFor(ctx, cfg.fieldsDeps, cfg.fieldsTool, result, filtered, len(r)) + } + callResult := utils.NewToolResultText(string(r)) if cfg.postProcess != nil { cfg.postProcess(ctx, result, callResult) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 24419ed24..fe1b0d5a3 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -184,6 +184,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GetFileContents(t), LegacyGetFileContents(t), ListCommits(t), + LegacyListCommits(t), SearchCode(t), LegacySearchCode(t), SearchCommits(t), @@ -193,6 +194,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { ListTags(t), GetTag(t), ListReleases(t), + LegacyListReleases(t), GetLatestRelease(t), GetReleaseByTag(t), CreateOrUpdateFile(t), @@ -212,7 +214,9 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { // Issue tools IssueRead(t), SearchIssues(t), + LegacySearchIssues(t), ListIssues(t), + LegacyListIssues(t), ListIssueTypes(t), ListIssueFields(t), IssueWrite(t), @@ -230,7 +234,9 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { // Pull request tools PullRequestRead(t), ListPullRequests(t), + LegacyListPullRequests(t), SearchPullRequests(t), + LegacySearchPullRequests(t), MergePullRequest(t), UpdatePullRequestBranch(t), CreatePullRequest(t),