From f68771e91144c3129b957b131d5054eb9d57ba4c Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 22:40:52 +0200 Subject: [PATCH 1/2] feat(issues): gate issue-fields features behind remote_mcp_issue_fields flag Gates the recently merged issue-fields work (list_issue_fields tool, field_values enrichment on list_issues/search_issues, and field_filters input on list_issues) behind a new feature flag, also enabled in insiders mode. - list_issues splits into two same-named registrations: the field-aware variant requires the flag, while LegacyListIssues (FeatureFlagDisable) preserves the prior schema and GraphQL selection set so disabled callers don't pay the extra wire/server cost. - search_issues skips the field-values lookup when the flag is off. - list_issue_fields requires the flag to be registered at all. - Adopts _ff_.snap naming for flagged toolsnap variants so same-named duplicates each get a distinct snapshot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 12 + pkg/github/__toolsnaps__/list_issues.snap | 21 -- ...ist_issues_ff_remote_mcp_issue_fields.snap | 92 +++++ pkg/github/feature_flags.go | 7 + pkg/github/issue_fields.go | 5 +- pkg/github/issues.go | 332 +++++++++++++++++- pkg/github/issues_test.go | 97 ++++- pkg/github/minimal_types.go | 45 +++ pkg/github/tools.go | 1 + 9 files changed, 584 insertions(+), 28 deletions(-) create mode 100644 pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields.snap diff --git a/README.md b/README.md index 6d29649658..cc147e9143 100644 --- a/README.md +++ b/README.md @@ -909,6 +909,18 @@ The following sets of tools are available: - `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_issues** - List issues + - **Required OAuth Scopes**: `repo` + - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) + - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (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) + - **remove_sub_issue** - Remove Sub-Issue - **Required OAuth Scopes**: `repo` - `issue_number`: The parent issue number (number, required) diff --git a/pkg/github/__toolsnaps__/list_issues.snap b/pkg/github/__toolsnaps__/list_issues.snap index b1d1c7a21d..a4be59bb0c 100644 --- a/pkg/github/__toolsnaps__/list_issues.snap +++ b/pkg/github/__toolsnaps__/list_issues.snap @@ -18,27 +18,6 @@ ], "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" - }, "labels": { "description": "Filter by labels", "items": { diff --git a/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields.snap b/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields.snap new file mode 100644 index 0000000000..b1d1c7a21d --- /dev/null +++ b/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields.snap @@ -0,0 +1,92 @@ +{ + "annotations": { + "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 endCursor from the previous page's PageInfo for GraphQL APIs.", + "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" + }, + "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/feature_flags.go b/pkg/github/feature_flags.go index 19399e7acf..6f04be7f15 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -11,6 +11,11 @@ const FeatureFlagCSVOutput = "csv_output" // FeatureFlagIFCLabels is the feature flag name for IFC security labels in tool results. const FeatureFlagIFCLabels = "ifc_labels" +// FeatureFlagIssueFields is the feature flag name for Issues 2.0 custom field +// support: the list_issue_fields tool, the field_filters input on list_issues, +// and field_values enrichment in list_issues / search_issues output. +const FeatureFlagIssueFields = "remote_mcp_issue_fields" + // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. @@ -18,6 +23,7 @@ const FeatureFlagIFCLabels = "ifc_labels" var AllowedFeatureFlags = []string{ MCPAppsFeatureFlag, FeatureFlagCSVOutput, + FeatureFlagIssueFields, FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular, } @@ -30,6 +36,7 @@ var InsidersFeatureFlags = []string{ MCPAppsFeatureFlag, FeatureFlagCSVOutput, FeatureFlagIFCLabels, + FeatureFlagIssueFields, } // FeatureFlags defines runtime feature toggles that adjust tool behavior. diff --git a/pkg/github/issue_fields.go b/pkg/github/issue_fields.go index 70f1a7c510..a7b7c429de 100644 --- a/pkg/github/issue_fields.go +++ b/pkg/github/issue_fields.go @@ -95,8 +95,9 @@ type issueFieldsOrgQuery struct { } // ListIssueFields creates a tool to list issue field definitions for a repository or organization. +// Gated by FeatureFlagIssueFields: the tool is only registered when the flag is on. func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool { - return NewTool( + st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "list_issue_fields", @@ -148,6 +149,8 @@ func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool return utils.NewToolResultText(string(r)), nil, nil }) + st.FeatureFlagEnable = FeatureFlagIssueFields + return st } // fetchIssueFields returns the issue field definitions for the given owner. diff --git a/pkg/github/issues.go b/pkg/github/issues.go index e56e793a46..0074bbd581 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -280,6 +280,123 @@ func getIssueQueryType(hasLabels bool, hasSince bool) any { } } +// --- Legacy list_issues GraphQL types --- +// +// These mirror the pre-Issues-2.0 shape of the list_issues query and exist solely +// to back the FeatureFlagIssueFields-disabled variant of the tool. They omit the +// IssueFieldValues selection and the filterBy: {issueFieldValues: ...} clause so +// the request does not depend on server-side issue_fields GraphQL features and +// does not pay the wire/server cost of fetching custom field values when the flag +// is off. Delete this whole block (and its callers) when FeatureFlagIssueFields +// is removed. + +type LegacyIssueFragment struct { + Number githubv4.Int + Title githubv4.String + Body githubv4.String + State githubv4.String + DatabaseID int64 + + Author struct { + Login githubv4.String + } + CreatedAt githubv4.DateTime + UpdatedAt githubv4.DateTime + Labels struct { + Nodes []struct { + Name githubv4.String + ID githubv4.String + Description githubv4.String + } + } `graphql:"labels(first: 100)"` + Comments struct { + TotalCount githubv4.Int + } `graphql:"comments"` +} + +type LegacyIssueQueryFragment struct { + Nodes []LegacyIssueFragment `graphql:"nodes"` + PageInfo struct { + HasNextPage githubv4.Boolean + HasPreviousPage githubv4.Boolean + StartCursor githubv4.String + EndCursor githubv4.String + } + TotalCount int +} + +type LegacyIssueQueryResult interface { + GetLegacyIssueFragment() LegacyIssueQueryFragment + GetIsPrivate() bool +} + +type LegacyListIssuesQuery struct { + Repository struct { + Issues LegacyIssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + IsPrivate githubv4.Boolean + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type LegacyListIssuesQueryTypeWithLabels struct { + Repository struct { + Issues LegacyIssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction})"` + IsPrivate githubv4.Boolean + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type LegacyListIssuesQueryWithSince struct { + Repository struct { + Issues LegacyIssueQueryFragment `graphql:"issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + IsPrivate githubv4.Boolean + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +type LegacyListIssuesQueryTypeWithLabelsWithSince struct { + Repository struct { + Issues LegacyIssueQueryFragment `graphql:"issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {since: $since})"` + IsPrivate githubv4.Boolean + } `graphql:"repository(owner: $owner, name: $repo)"` +} + +func (q *LegacyListIssuesQuery) GetLegacyIssueFragment() LegacyIssueQueryFragment { + return q.Repository.Issues +} +func (q *LegacyListIssuesQuery) GetIsPrivate() bool { return bool(q.Repository.IsPrivate) } + +func (q *LegacyListIssuesQueryTypeWithLabels) GetLegacyIssueFragment() LegacyIssueQueryFragment { + return q.Repository.Issues +} +func (q *LegacyListIssuesQueryTypeWithLabels) GetIsPrivate() bool { + return bool(q.Repository.IsPrivate) +} + +func (q *LegacyListIssuesQueryWithSince) GetLegacyIssueFragment() LegacyIssueQueryFragment { + return q.Repository.Issues +} +func (q *LegacyListIssuesQueryWithSince) GetIsPrivate() bool { + return bool(q.Repository.IsPrivate) +} + +func (q *LegacyListIssuesQueryTypeWithLabelsWithSince) GetLegacyIssueFragment() LegacyIssueQueryFragment { + return q.Repository.Issues +} +func (q *LegacyListIssuesQueryTypeWithLabelsWithSince) GetIsPrivate() bool { + return bool(q.Repository.IsPrivate) +} + +func getLegacyIssueQueryType(hasLabels bool, hasSince bool) any { + switch { + case hasLabels && hasSince: + return &LegacyListIssuesQueryTypeWithLabelsWithSince{} + case hasLabels: + return &LegacyListIssuesQueryTypeWithLabels{} + case hasSince: + return &LegacyListIssuesQueryWithSince{} + default: + return &LegacyListIssuesQuery{} + } +} + // IssueRead creates a tool to get details of a specific issue in a GitHub repository. func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ @@ -1262,7 +1379,7 @@ func searchIssuesHandler(ctx context.Context, deps ToolDependencies, args map[st } var fieldValuesByID map[string][]MinimalIssueFieldValue - if len(result.Issues) > 0 { + if deps.IsFeatureEnabled(ctx, FeatureFlagIssueFields) && len(result.Issues) > 0 { gqlClient, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr(errorPrefix+": failed to get GitHub GraphQL client", err), nil @@ -1700,7 +1817,11 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return utils.NewToolResultText(string(r)), nil } -// ListIssues creates a tool to list and filter repository issues +// ListIssues creates a tool to list and filter repository issues. This variant is +// gated by FeatureFlagIssueFields and exposes the Issues 2.0 field_filters input +// plus field_values output enrichment. When the flag is off, LegacyListIssues is +// served instead. Both registrations share the tool name "list_issues" and rely on +// the inventory's feature-flag filter to make exactly one active at a time. func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", @@ -1762,7 +1883,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } WithCursorPagination(schema) - return NewTool( + st := NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "list_issues", @@ -1962,6 +2083,211 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } return result, nil, nil }) + st.FeatureFlagEnable = FeatureFlagIssueFields + return st +} + +// LegacyListIssues is the FeatureFlagIssueFields-disabled variant of list_issues. +// It exposes the pre-Issues-2.0 schema (no field_filters) and uses a GraphQL query +// path that does not select issueFieldValues or pass the issue_fields filter, so +// the request does not depend on server-side issue_fields features and does not pay +// for custom field values when the flag is off. Both this and ListIssues register +// under the tool name "list_issues"; exactly one is active for any given request +// thanks to mutually exclusive FeatureFlagEnable / FeatureFlagDisable annotations. +// Delete this function (and the rest of the Legacy* block) when the flag is removed. +func LegacyListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + schema := &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "state": { + Type: "string", + Description: "Filter by state, by default both open and closed issues are returned when not provided", + Enum: []any{"OPEN", "CLOSED"}, + }, + "labels": { + Type: "array", + Description: "Filter by labels", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, + "orderBy": { + Type: "string", + Description: "Order issues by field. If provided, the 'direction' also needs to be provided.", + Enum: []any{"CREATED_AT", "UPDATED_AT", "COMMENTS"}, + }, + "direction": { + Type: "string", + Description: "Order direction. If provided, the 'orderBy' also needs to be provided.", + Enum: []any{"ASC", "DESC"}, + }, + "since": { + Type: "string", + Description: "Filter by date (ISO 8601 timestamp)", + }, + }, + Required: []string{"owner", "repo"}, + } + WithCursorPagination(schema) + + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "list_issues", + Description: t("TOOL_LIST_ISSUES_DESCRIPTION", "List issues in a GitHub repository. For pagination, use the 'endCursor' from the previous response's 'pageInfo' in the 'after' parameter."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_ISSUES_USER_TITLE", "List issues"), + ReadOnlyHint: true, + }, + InputSchema: schema, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + state, err := OptionalParam[string](args, "state") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + state = strings.ToUpper(state) + var states []githubv4.IssueState + switch state { + case "OPEN", "CLOSED": + states = []githubv4.IssueState{githubv4.IssueState(state)} + default: + states = []githubv4.IssueState{githubv4.IssueStateOpen, githubv4.IssueStateClosed} + } + + labels, err := OptionalStringArrayParam(args, "labels") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + orderBy, err := OptionalParam[string](args, "orderBy") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + direction, err := OptionalParam[string](args, "direction") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + orderBy = strings.ToUpper(orderBy) + switch orderBy { + case "CREATED_AT", "UPDATED_AT", "COMMENTS": + default: + orderBy = "CREATED_AT" + } + direction = strings.ToUpper(direction) + switch direction { + case "ASC", "DESC": + default: + direction = "DESC" + } + + since, err := OptionalParam[string](args, "since") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + var sinceTime time.Time + var hasSince bool + if since != "" { + sinceTime, err = parseISOTimestamp(since) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to list issues: %s", err.Error())), nil, nil + } + hasSince = true + } + hasLabels := len(labels) > 0 + + pagination, err := OptionalCursorPaginationParams(args) + if err != nil { + return nil, nil, err + } + if _, pageProvided := args["page"]; pageProvided { + return utils.NewToolResultError("This tool uses cursor-based pagination. Use the 'after' parameter with the 'endCursor' value from the previous response instead of 'page'."), nil, nil + } + _, perPageProvided := args["perPage"] + paginationExplicit := perPageProvided + paginationParams, err := pagination.ToGraphQLParams() + if err != nil { + return nil, nil, err + } + if !paginationExplicit { + defaultFirst := int32(DefaultGraphQLPageSize) + paginationParams.First = &defaultFirst + } + + client, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil + } + + vars := map[string]any{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "states": states, + "orderBy": githubv4.IssueOrderField(orderBy), + "direction": githubv4.OrderDirection(direction), + "first": githubv4.Int(*paginationParams.First), + } + if paginationParams.After != nil { + vars["after"] = githubv4.String(*paginationParams.After) + } else { + vars["after"] = (*githubv4.String)(nil) + } + if hasLabels { + labelStrings := make([]githubv4.String, len(labels)) + for i, label := range labels { + labelStrings[i] = githubv4.String(label) + } + vars["labels"] = labelStrings + } + if hasSince { + vars["since"] = githubv4.DateTime{Time: sinceTime} + } + + issueQuery := getLegacyIssueQueryType(hasLabels, hasSince) + if err := client.Query(ctx, issueQuery, vars); err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse( + ctx, + "failed to list issues", + err, + ), nil, nil + } + + var resp MinimalIssuesResponse + var isPrivate bool + if queryResult, ok := issueQuery.(LegacyIssueQueryResult); ok { + resp = convertLegacyToMinimalIssuesResponse(queryResult.GetLegacyIssueFragment()) + isPrivate = queryResult.GetIsPrivate() + } + + result := MarshalledTextResult(resp) + if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { + if result.Meta == nil { + result.Meta = mcp.Meta{} + } + result.Meta["ifc"] = ifc.LabelListIssues(isPrivate) + } + return result, nil, nil + }) + st.FeatureFlagDisable = FeatureFlagIssueFields + return st } // rawFieldFilter is the user-supplied {field_name, value} pair before type resolution. diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 4f08b72140..3bac597225 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1082,8 +1082,9 @@ func Test_SearchIssues_FieldValuesEnrichment(t *testing.T) { gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) deps := BaseDeps{ - Client: mustNewGHClient(t, restClient), - GQLClient: gqlClient, + Client: mustNewGHClient(t, restClient), + GQLClient: gqlClient, + featureChecker: featureCheckerFor(FeatureFlagIssueFields), } handler := serverTool.Handler(deps) @@ -1446,7 +1447,8 @@ 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.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFields, tool)) + require.Equal(t, FeatureFlagIssueFields, serverTool.FeatureFlagEnable) assert.Equal(t, "list_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -2363,6 +2365,95 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { }) } +func Test_LegacyListIssues_Definition(t *testing.T) { + serverTool := LegacyListIssues(translations.NullTranslationHelper) + tool := serverTool.Tool + + // LegacyListIssues claims the base tool name "list_issues" and produces the + // FeatureFlagIssueFields-disabled schema (no field_filters). It owns the + // canonical list_issues.snap; the FeatureFlagIssueFields-enabled variant + // owns list_issues_ff_.snap. + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.Equal(t, "list_issues", tool.Name) + require.Equal(t, FeatureFlagIssueFields, serverTool.FeatureFlagDisable) + require.Empty(t, serverTool.FeatureFlagEnable) + + props := tool.InputSchema.(*jsonschema.Schema).Properties + assert.Contains(t, props, "owner") + assert.Contains(t, props, "repo") + assert.Contains(t, props, "state") + assert.Contains(t, props, "labels") + assert.Contains(t, props, "since") + assert.NotContains(t, props, "field_filters", "legacy list_issues must not advertise field_filters") +} + +func Test_LegacyListIssues_OmitsFieldValuesAndFilters(t *testing.T) { + t.Parallel() + + serverTool := LegacyListIssues(translations.NullTranslationHelper) + + mockIssues := []map[string]any{ + { + "number": 7, + "title": "Legacy issue", + "body": "body", + "state": "OPEN", + "databaseId": 7, + "createdAt": "2026-01-01T00:00:00Z", + "updatedAt": "2026-01-01T00:00:00Z", + "author": map[string]any{"login": "octocat"}, + "labels": map[string]any{"nodes": []map[string]any{}}, + "comments": map[string]any{"totalCount": 0}, + }, + } + pageInfo := map[string]any{ + "hasNextPage": false, + "hasPreviousPage": false, + "startCursor": "c1", + "endCursor": "c1", + } + + // The legacy query must NOT reference issueFieldValues (neither in the selection + // set nor in filterBy). The matcher's query string therefore omits both. + const legacyQuery = "query($after:String$direction:OrderDirection!$first:Int!$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}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + vars := map[string]any{ + "owner": "owner", + "repo": "repo", + "states": []any{"OPEN", "CLOSED"}, + "orderBy": "CREATED_AT", + "direction": "DESC", + "first": float64(30), + "after": nil, + } + response := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "isPrivate": false, + "issues": map[string]any{ + "nodes": mockIssues, + "pageInfo": pageInfo, + "totalCount": 1, + }, + }, + }) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher(legacyQuery, vars, response))) + + deps := BaseDeps{GQLClient: gqlClient} + 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, "expected non-error result; got: %v", getTextResult(t, result).Text) + + var resp MinimalIssuesResponse + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &resp)) + require.Len(t, resp.Issues, 1) + assert.Equal(t, 7, resp.Issues[0].Number) + assert.Nil(t, resp.Issues[0].FieldValues, "legacy list_issues must not return field_values") +} + func Test_UpdateIssue(t *testing.T) { // Verify tool definition serverTool := IssueWrite(translations.NullTranslationHelper) diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index bad5196a9b..02309db45b 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -525,6 +525,51 @@ func convertToMinimalIssuesResponse(fragment IssueQueryFragment) MinimalIssuesRe } } +// legacyFragmentToMinimalIssue converts the FeatureFlagIssueFields-disabled +// LegacyIssueFragment into a MinimalIssue. MinimalIssue.FieldValues is left +// nil so omitempty drops it from JSON output. Delete with the rest of the +// Legacy* block when the flag is removed. +func legacyFragmentToMinimalIssue(fragment LegacyIssueFragment) MinimalIssue { + m := MinimalIssue{ + Number: int(fragment.Number), + Title: sanitize.Sanitize(string(fragment.Title)), + Body: sanitize.Sanitize(string(fragment.Body)), + State: string(fragment.State), + Comments: int(fragment.Comments.TotalCount), + CreatedAt: fragment.CreatedAt.Format(time.RFC3339), + UpdatedAt: fragment.UpdatedAt.Format(time.RFC3339), + User: &MinimalUser{ + Login: string(fragment.Author.Login), + }, + } + + for _, label := range fragment.Labels.Nodes { + m.Labels = append(m.Labels, string(label.Name)) + } + + return m +} + +// convertLegacyToMinimalIssuesResponse mirrors convertToMinimalIssuesResponse for +// the FeatureFlagIssueFields-disabled list_issues variant. +func convertLegacyToMinimalIssuesResponse(fragment LegacyIssueQueryFragment) MinimalIssuesResponse { + minimalIssues := make([]MinimalIssue, 0, len(fragment.Nodes)) + for _, issue := range fragment.Nodes { + minimalIssues = append(minimalIssues, legacyFragmentToMinimalIssue(issue)) + } + + return MinimalIssuesResponse{ + Issues: minimalIssues, + TotalCount: fragment.TotalCount, + PageInfo: MinimalPageInfo{ + HasNextPage: bool(fragment.PageInfo.HasNextPage), + HasPreviousPage: bool(fragment.PageInfo.HasPreviousPage), + StartCursor: string(fragment.PageInfo.StartCursor), + EndCursor: string(fragment.PageInfo.EndCursor), + }, + } +} + func convertToMinimalIssueComment(comment *github.IssueComment) MinimalIssueComment { m := MinimalIssueComment{ ID: comment.GetID(), diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 70dfab8d9e..49edb00fff 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -204,6 +204,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { IssueRead(t), SearchIssues(t), ListIssues(t), + LegacyListIssues(t), ListIssueTypes(t), ListIssueFields(t), IssueWrite(t), From 0bc4ae45b4a6105697f9aa66938917ddf33e77bb Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 22:54:12 +0200 Subject: [PATCH 2/2] fix: address PR review on issue-fields gating - docs generator: install a no-flags feature checker so README reflects the default user experience (tools enabled with no special flags), fixing duplicate `list_issues` and removing granular/flagged-only tools that were never meant to appear in the default docs. - csv_output: drop the FeatureFlagEnable/Disable exclusion in isCSVOutputTool. Wrapping happens before the per-request flag filter picks the live variant, so flag-gated list_* tools wrap safely; this restores CSV conversion for `list_issues` and enables it for `list_issue_fields` when both flags are on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 187 ------------------------- cmd/github-mcp-server/generate_docs.go | 17 ++- pkg/github/csv_output.go | 10 +- pkg/github/csv_output_test.go | 20 +++ 4 files changed, 41 insertions(+), 193 deletions(-) diff --git a/README.md b/README.md index cc147e9143..b387b61f15 100644 --- a/README.md +++ b/README.md @@ -829,21 +829,6 @@ The following sets of tools are available: - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) -- **add_sub_issue** - Add Sub-Issue - - **Required OAuth Scopes**: `repo` - - `issue_number`: The parent issue number (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `replace_parent`: If true, reparent the sub-issue if it already has a parent (boolean, optional) - - `repo`: Repository name (string, required) - - `sub_issue_id`: The ID of the sub-issue to add. ID is not the same as issue number (number, required) - -- **create_issue** - Create Issue - - **Required OAuth Scopes**: `repo` - - `body`: Issue body content (optional) (string, optional) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - `title`: Issue title (string, required) - - **get_label** - Get a specific label from a repository - **Required OAuth Scopes**: `repo` - `name`: Label name. (string, required) @@ -885,30 +870,11 @@ The following sets of tools are available: - `title`: Issue title (string, optional) - `type`: Type of this issue. Only use if the repository has issue types configured. Use list_issue_types tool to get valid type values for the organization. If the repository doesn't support issue types, omit this parameter. (string, optional) -- **list_issue_fields** - List issue fields - - **Required OAuth Scopes**: `repo`, `read:org` - - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `repo`, `write:org` - - `owner`: The account owner of the repository or organization. The name is not case sensitive. (string, required) - - `repo`: The name of the repository. When provided, returns fields for this specific repository (inherited from its organization). When omitted, returns org-level fields directly. (string, optional) - - **list_issue_types** - List available issue types - **Required OAuth Scopes**: `read:org` - **Accepted OAuth Scopes**: `admin:org`, `read:org`, `write:org` - `owner`: The organization owner of the repository (string, required) -- **list_issues** - List issues - - **Required OAuth Scopes**: `repo` - - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (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) - - `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_issues** - List issues - **Required OAuth Scopes**: `repo` - `after`: Cursor for pagination. Use the endCursor from the previous page's PageInfo for GraphQL APIs. (string, optional) @@ -921,22 +887,6 @@ The following sets of tools are available: - `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) -- **remove_sub_issue** - Remove Sub-Issue - - **Required OAuth Scopes**: `repo` - - `issue_number`: The parent issue number (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - `sub_issue_id`: The ID of the sub-issue to remove. ID is not the same as issue number (number, required) - -- **reprioritize_sub_issue** - Reprioritize Sub-Issue - - **Required OAuth Scopes**: `repo` - - `after_id`: The ID of the sub-issue to place this after (either after_id OR before_id should be specified) (number, optional) - - `before_id`: The ID of the sub-issue to place this before (either after_id OR before_id should be specified) (number, optional) - - `issue_number`: The parent issue number (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - `sub_issue_id`: The ID of the sub-issue to reorder. ID is not the same as issue number (number, required) - - **search_issues** - Search issues - **Required OAuth Scopes**: `repo` - `order`: Sort order (string, optional) @@ -947,13 +897,6 @@ The following sets of tools are available: - `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) -- **set_issue_fields** - Set Issue Fields - - **Required OAuth Scopes**: `repo` - - `fields`: Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value. (object[], required) - - `issue_number`: The issue number to update (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - **sub_issue_write** - Change sub-issue - **Required OAuth Scopes**: `repo` - `after_id`: The ID of the sub-issue to be prioritized after (either after_id OR before_id should be specified) (number, optional) @@ -970,57 +913,6 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - `sub_issue_id`: The ID of the sub-issue to add. ID is not the same as issue number (number, required) -- **update_issue_assignees** - Update Issue Assignees - - **Required OAuth Scopes**: `repo` - - `assignees`: GitHub usernames to assign to this issue (string[], required) - - `issue_number`: The issue number to update (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - -- **update_issue_body** - Update Issue Body - - **Required OAuth Scopes**: `repo` - - `body`: The new body content for the issue (string, required) - - `issue_number`: The issue number to update (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - -- **update_issue_labels** - Update Issue Labels - - **Required OAuth Scopes**: `repo` - - `issue_number`: The issue number to update (number, required) - - `labels`: Labels to apply to this issue. ([], required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - -- **update_issue_milestone** - Update Issue Milestone - - **Required OAuth Scopes**: `repo` - - `issue_number`: The issue number to update (number, required) - - `milestone`: The milestone number to set on the issue (integer, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - -- **update_issue_state** - Update Issue State - - **Required OAuth Scopes**: `repo` - - `issue_number`: The issue number to update (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - `state`: The new state for the issue (string, required) - - `state_reason`: The reason for the state change (only for closed state) (string, optional) - -- **update_issue_title** - Update Issue Title - - **Required OAuth Scopes**: `repo` - - `issue_number`: The issue number to update (number, required) - - `owner`: Repository owner (username or organization) (string, required) - - `repo`: Repository name (string, required) - - `title`: The new title for the issue (string, required) - -- **update_issue_type** - Update Issue Type - - **Required OAuth Scopes**: `repo` - - `issue_number`: The issue number to update (number, required) - - `issue_type`: The issue type to set (string, required) - - `owner`: Repository owner (username or organization) (string, required) - - `rationale`: One concise sentence explaining what specifically about the issue led you to choose this type. State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature). (string, optional) - - `repo`: Repository name (string, required) -
@@ -1173,19 +1065,6 @@ The following sets of tools are available: - `startSide`: For multi-line comments, the starting side of the diff that the comment applies to. LEFT indicates the previous state, RIGHT indicates the new state (string, optional) - `subjectType`: The level at which the comment is targeted (string, required) -- **add_pull_request_review_comment** - Add Pull Request Review Comment - - **Required OAuth Scopes**: `repo` - - `body`: The comment body (string, required) - - `line`: The line number in the diff to comment on (optional) (number, optional) - - `owner`: Repository owner (username or organization) (string, required) - - `path`: The relative path of the file to comment on (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - `side`: The side of the diff to comment on (optional) (string, optional) - - `startLine`: The start line of a multi-line comment (optional) (number, optional) - - `startSide`: The start side of a multi-line comment (optional) (string, optional) - - `subjectType`: The subject type of the comment (string, required) - - **add_reply_to_pull_request_comment** - Add reply to pull request comment - **Required OAuth Scopes**: `repo` - `body`: The text of the reply (string, required) @@ -1205,21 +1084,6 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - `title`: PR title (string, required) -- **create_pull_request_review** - Create Pull Request Review - - **Required OAuth Scopes**: `repo` - - `body`: The review body text (optional) (string, optional) - - `commitID`: The SHA of the commit to review (optional, defaults to latest) (string, optional) - - `event`: The review action to perform. If omitted, creates a pending review. (string, optional) - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - -- **delete_pending_pull_request_review** - Delete Pending Pull Request Review - - **Required OAuth Scopes**: `repo` - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - **list_pull_requests** - List pull requests - **Required OAuth Scopes**: `repo` - `base`: Filter by base branch (string, optional) @@ -1272,17 +1136,6 @@ The following sets of tools are available: - `repo`: Repository name (string, required) - `threadId`: The node ID of the review thread (e.g., PRRT_kwDOxxx). Required for resolve_thread and unresolve_thread methods. Get thread IDs from pull_request_read with method get_review_comments. (string, optional) -- **request_pull_request_reviewers** - Request Pull Request Reviewers - - **Required OAuth Scopes**: `repo` - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - `reviewers`: GitHub usernames to request reviews from (string[], required) - -- **resolve_review_thread** - Resolve Review Thread - - **Required OAuth Scopes**: `repo` - - `threadID`: The node ID of the review thread to resolve (e.g., PRRT_kwDOxxx) (string, required) - - **search_pull_requests** - Search pull requests - **Required OAuth Scopes**: `repo` - `order`: Sort order (string, optional) @@ -1293,18 +1146,6 @@ The following sets of tools are available: - `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) -- **submit_pending_pull_request_review** - Submit Pending Pull Request Review - - **Required OAuth Scopes**: `repo` - - `body`: The review body text (optional) (string, optional) - - `event`: The review action to perform (string, required) - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - -- **unresolve_review_thread** - Unresolve Review Thread - - **Required OAuth Scopes**: `repo` - - `threadID`: The node ID of the review thread to unresolve (e.g., PRRT_kwDOxxx) (string, required) - - **update_pull_request** - Edit pull request - **Required OAuth Scopes**: `repo` - `base`: New base branch name (string, optional) @@ -1318,13 +1159,6 @@ The following sets of tools are available: - `state`: New state (string, optional) - `title`: New title (string, optional) -- **update_pull_request_body** - Update Pull Request Body - - **Required OAuth Scopes**: `repo` - - `body`: The new body content for the pull request (string, required) - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - **update_pull_request_branch** - Update pull request branch - **Required OAuth Scopes**: `repo` - `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional) @@ -1332,27 +1166,6 @@ The following sets of tools are available: - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) -- **update_pull_request_draft_state** - Update Pull Request Draft State - - **Required OAuth Scopes**: `repo` - - `draft`: Set to true to convert to draft, false to mark as ready for review (boolean, required) - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - -- **update_pull_request_state** - Update Pull Request State - - **Required OAuth Scopes**: `repo` - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - `state`: The new state for the pull request (string, required) - -- **update_pull_request_title** - Update Pull Request Title - - **Required OAuth Scopes**: `repo` - - `owner`: Repository owner (username or organization) (string, required) - - `pullNumber`: The pull request number (number, required) - - `repo`: Repository name (string, required) - - `title`: The new title for the pull request (string, required) -
diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 7a97e4f661..7295c9ccf7 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -29,6 +29,12 @@ func init() { rootCmd.AddCommand(generateDocsCmd) } +// noFeatureFlagsChecker reports every feature flag as disabled. It models the +// default user experience used by the generated documentation. +func noFeatureFlagsChecker(_ context.Context, _ string) (bool, error) { + return false, nil +} + func generateAllDocs() error { for _, doc := range []struct { path string @@ -51,9 +57,16 @@ func generateReadmeDocs(readmePath string) error { // Create translation helper t, _ := translations.TranslationHelper() - // (not available to regular users) while including tools with FeatureFlagDisable. + // The README documents the default user experience: tools that are + // enabled with no special flags set. Installing a checker that reports + // every flag as disabled excludes tools gated by FeatureFlagEnable and + // keeps the legacy variants of tools gated by FeatureFlagDisable, so + // flag-gated duplicates don't appear twice. // Build() can only fail if WithTools specifies invalid tools - not used here - r, _ := github.NewInventory(t).WithToolsets([]string{"all"}).Build() + r, _ := github.NewInventory(t). + WithToolsets([]string{"all"}). + WithFeatureChecker(noFeatureFlagsChecker). + Build() // Generate toolsets documentation toolsetsDoc := generateToolsetsDoc(r) diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index cb70e32d77..6acb8b2fdb 100644 --- a/pkg/github/csv_output.go +++ b/pkg/github/csv_output.go @@ -56,14 +56,16 @@ func withCSVOutput(tools []inventory.ServerTool) []inventory.ServerTool { return tools } +// isCSVOutputTool reports whether the given tool should have its handler +// wrapped to honor the csv_output feature flag. Wrapping happens at slice +// construction time, before the per-request feature-flag filter chooses which +// variant of a flag-gated tool to register, so flag-gated list_* tools are +// included on equal footing — only the live variant ever runs at request time. func isCSVOutputTool(tool inventory.ServerTool) bool { if !tool.Toolset.Default { return false } - if !strings.HasPrefix(tool.Tool.Name, "list_") { - return false - } - return tool.FeatureFlagEnable == "" && tool.FeatureFlagDisable == "" + return strings.HasPrefix(tool.Tool.Name, "list_") } func wrapHandlerWithCSVOutput(next inventory.HandlerFunc) inventory.HandlerFunc { diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go index d0bef38938..246902d498 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -38,6 +38,26 @@ func TestCSVOutputAppliedToDefaultListTools(t *testing.T) { } } +func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) { + enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`) + enabledOnly.FeatureFlagEnable = FeatureFlagIssueFields + disabledOnly := testCSVOutputTool("list_legacy_things", `[{"number":2}]`) + disabledOnly.FeatureFlagDisable = FeatureFlagIssueFields + + tools := withCSVOutput([]inventory.ServerTool{enabledOnly, disabledOnly}) + require.Len(t, tools, 2) + + // Both flag-gated variants get the CSV wrapper; the per-request flag filter + // decides which one actually registers, and the runtime csv_output check + // decides whether the wrapper converts the response. + deps := newCSVOutputTestDeps(true) + for _, tool := range tools { + result, err := tool.Handler(deps)(ContextWithDeps(context.Background(), deps), testCSVOutputRequest()) + require.NoError(t, err) + assert.Contains(t, textResult(t, result), "number\n") + } +} + func TestCSVOutputOnlyAppliesToDefaultToolsets(t *testing.T) { nonDefaultListTool := testCSVOutputToolWithToolset("list_discussions", `[{"number":1}]`, ToolsetMetadataDiscussions)