From 4c926eacd9c14e3f2fdf140a7160d7399c7ae85f Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Mon, 11 May 2026 12:38:19 +0100 Subject: [PATCH 1/8] Add CSV output for list tools under insiders mode --- cmd/github-mcp-server/main.go | 8 + docs/insiders-features.md | 25 ++ internal/ghmcp/server.go | 5 +- pkg/github/context_tools.go | 2 +- pkg/github/context_tools_test.go | 33 ++- pkg/github/csv_output.go | 412 +++++++++++++++++++++++++++ pkg/github/csv_output_test.go | 415 ++++++++++++++++++++++++++++ pkg/github/dependencies.go | 1 - pkg/github/feature_flags.go | 28 +- pkg/github/feature_flags_test.go | 109 ++------ pkg/github/issues.go | 4 +- pkg/github/issues_test.go | 16 +- pkg/github/pullrequests.go | 4 +- pkg/github/pullrequests_test.go | 10 +- pkg/github/server.go | 2 +- pkg/github/server_test.go | 2 - pkg/github/tools.go | 4 +- pkg/github/tools_validation_test.go | 31 +++ pkg/github/ui_embed.go | 2 +- pkg/http/handler.go | 9 +- pkg/http/handler_test.go | 29 +- pkg/http/middleware/cors.go | 1 + pkg/http/server.go | 21 +- pkg/http/server_test.go | 36 ++- pkg/inventory/filters.go | 96 +++++-- 25 files changed, 1137 insertions(+), 168 deletions(-) create mode 100644 pkg/github/csv_output.go create mode 100644 pkg/github/csv_output_test.go diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index ec948ab6e0..ab8b27bb3c 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -126,6 +126,13 @@ var ( } } + var enabledFeatures []string + if viper.IsSet("features") { + if err := viper.UnmarshalKey("features", &enabledFeatures); err != nil { + return fmt.Errorf("failed to unmarshal features: %w", err) + } + } + ttl := viper.GetDuration("repo-access-cache-ttl") httpConfig := ghhttp.ServerConfig{ Version: version, @@ -144,6 +151,7 @@ var ( EnabledToolsets: enabledToolsets, EnabledTools: enabledTools, ExcludeTools: excludeTools, + EnabledFeatures: enabledFeatures, InsidersMode: viper.GetBool("insiders"), } diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 911257ae4f..59147c4f58 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -42,3 +42,28 @@ MCP Apps requires a host that supports the [MCP Apps extension](https://modelcon - **VS Code Insiders** — enable via the `chat.mcp.apps.enabled` setting - **Visual Studio Code** — enable via the `chat.mcp.apps.enabled` setting + +--- + +## CSV output for list tools + +CSV output mode returns supported list tool responses as CSV instead of JSON. This is intended to reduce response context for agents when scanning or summarising lists of GitHub data. + +CSV output applies only to tools in default toolsets whose names start with `list_`, such as `list_issues`, `list_pull_requests`, `list_commits`, and `list_branches`. It does not add new tools or expose a tool argument for selecting the format; the server controls the response format through the Insiders feature flag. + +### Format + +- Nested objects are flattened into dot-notation columns, for example `user.login`, `category.name`, or `head.ref`. +- Arrays are represented as compact single-cell values joined with `;`. +- `body` fields are whitespace-normalized so multiline Markdown does not expand a list response into many output lines. +- Response metadata present in wrapped responses, such as `pageInfo.*` and `totalCount`, is emitted as `#`-prefixed lines before the CSV rows, followed by a blank line. Tools that return a root JSON array do not include metadata preamble lines. + +### Enabling CSV output + +CSV output is enabled by Insiders Mode. For local development, it can also be enabled explicitly with the `csv_output` feature flag: + +```bash +github-mcp-server stdio --features csv_output +``` + +Because this changes list tool response shape, clients that require JSON list responses should avoid enabling this feature. diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 3ca249dd17..38106b6d9a 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -143,7 +143,6 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se cfg.Translator, github.FeatureFlags{ LockdownMode: cfg.LockdownMode, - InsidersMode: cfg.InsidersMode, }, cfg.ContentWindowSize, featureChecker, @@ -229,7 +228,7 @@ type StdioServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool - // InsidersMode indicates if we should enable experimental features + // InsidersMode expands to the curated set of feature flags enabled for insiders. InsidersMode bool // ExcludeTools is a list of tool names to disable regardless of other settings. @@ -345,7 +344,7 @@ func RunStdioServer(cfg StdioServerConfig) error { // createFeatureChecker returns a FeatureFlagChecker that resolves features // using the centralized ResolveFeatureFlags function. For the local server, -// features are resolved once at startup from --features CLI flag + insiders mode. +// features are resolved once at startup from --features CLI flag and insiders mode. func createFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { featureSet := github.ResolveFeatureFlags(enabledFeatures, insidersMode) return func(_ context.Context, flagName string) (bool, error) { diff --git a/pkg/github/context_tools.go b/pkg/github/context_tools.go index 191e562793..4008c2f4aa 100644 --- a/pkg/github/context_tools.go +++ b/pkg/github/context_tools.go @@ -106,7 +106,7 @@ func GetMe(t translations.TranslationHelperFunc) inventory.ServerTool { } result := MarshalledTextResult(minimalUser) - if deps.GetFlags(ctx).InsidersMode { + if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { if result.Meta == nil { result.Meta = mcp.Meta{} } diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 2b17be86d1..ade54aba17 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -139,7 +139,7 @@ func Test_GetMe(t *testing.T) { } } -func Test_GetMe_IFC_InsidersMode(t *testing.T) { +func Test_GetMe_IFC_FeatureFlag(t *testing.T) { t.Parallel() serverTool := GetMe(translations.NullTranslationHelper) @@ -153,11 +153,21 @@ func Test_GetMe_IFC_InsidersMode(t *testing.T) { GetUser: mockResponse(t, http.StatusOK, mockUser), }) - t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) { - deps := BaseDeps{ - Client: mustNewGHClient(t, mockedHTTPClient), - Flags: FeatureFlags{InsidersMode: false}, - } + depsWithIFCFeature := func(enabled bool) *BaseDeps { + return NewBaseDeps( + mustNewGHClient(t, mockedHTTPClient), nil, nil, nil, + translations.NullTranslationHelper, + FeatureFlags{}, + 0, + func(_ context.Context, flagName string) (bool, error) { + return flagName == FeatureFlagIFCLabels && enabled, nil + }, + stubExporters(), + ) + } + + t.Run("feature disabled omits ifc label from result meta", func(t *testing.T) { + deps := depsWithIFCFeature(false) handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{}) @@ -165,14 +175,11 @@ func Test_GetMe_IFC_InsidersMode(t *testing.T) { require.NoError(t, err) require.False(t, result.IsError) - assert.Nil(t, result.Meta, "result meta should be nil when insiders mode is disabled") + assert.Nil(t, result.Meta, "result meta should be nil when IFC labels are disabled") }) - t.Run("insiders mode enabled includes ifc label in result meta", func(t *testing.T) { - deps := BaseDeps{ - Client: mustNewGHClient(t, mockedHTTPClient), - Flags: FeatureFlags{InsidersMode: true}, - } + t.Run("feature enabled includes ifc label in result meta", func(t *testing.T) { + deps := depsWithIFCFeature(true) handler := serverTool.Handler(deps) request := createMCPRequest(map[string]any{}) @@ -180,7 +187,7 @@ func Test_GetMe_IFC_InsidersMode(t *testing.T) { require.NoError(t, err) require.False(t, result.IsError) - require.NotNil(t, result.Meta, "result meta should be set when insiders mode is enabled") + require.NotNil(t, result.Meta, "result meta should be set when IFC labels are enabled") ifcLabel, ok := result.Meta["ifc"] require.True(t, ok, "result meta should contain ifc key") diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go new file mode 100644 index 0000000000..5c06a1c8c7 --- /dev/null +++ b/pkg/github/csv_output.go @@ -0,0 +1,412 @@ +package github + +import ( + "bytes" + "context" + "encoding/csv" + "encoding/json" + "fmt" + "sort" + "strings" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// Ordered by preference when a response wrapper contains multiple arrays. +var primaryCSVRowKeys = []string{ + "items", + "issues", + "discussions", + "categories", + "labels", + "alerts", + "advisories", + "notifications", + "gists", + "repositories", + "commits", + "branches", + "tags", + "releases", + "users", + "teams", + "members", + "projects", + "nodes", +} + +type csvOutputDocument struct { + metadata map[string]string + rows []map[string]string +} + +func withCSVOutputVariants(tools []inventory.ServerTool) []inventory.ServerTool { + result := make([]inventory.ServerTool, 0, len(tools)) + for _, tool := range tools { + if !isCSVOutputTool(tool) { + result = append(result, tool) + continue + } + + jsonOnly := tool + jsonOnly.FeatureFlagDisable = FeatureFlagCSVOutput + result = append(result, jsonOnly) + + csvCapable := tool + csvCapable.FeatureFlagEnable = FeatureFlagCSVOutput + csvCapable.HandlerFunc = wrapHandlerWithCSVOutput(tool.HandlerFunc) + result = append(result, csvCapable) + } + return result +} + +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 == "" +} + +func wrapHandlerWithCSVOutput(next inventory.HandlerFunc) inventory.HandlerFunc { + return func(deps any) mcp.ToolHandler { + handler := next(deps) + return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + result, err := handler(ctx, req) + if err != nil || result == nil || result.IsError { + return result, err + } + + return convertJSONTextResultToCSV(result), nil + } + } +} + +func convertJSONTextResultToCSV(result *mcp.CallToolResult) *mcp.CallToolResult { + if len(result.Content) != 1 { + return utils.NewToolResultError("failed to convert response to CSV: expected a single text content response") + } + + text, ok := result.Content[0].(*mcp.TextContent) + if !ok { + return utils.NewToolResultError("failed to convert response to CSV: expected a text content response") + } + + csvText, err := jsonTextToCSV(text.Text) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to convert response to CSV", err) + } + + result.Content = []mcp.Content{&mcp.TextContent{Text: csvText}} + result.StructuredContent = nil + return result +} + +func jsonTextToCSV(text string) (string, error) { + decoder := json.NewDecoder(strings.NewReader(text)) + decoder.UseNumber() + + var value any + if err := decoder.Decode(&value); err != nil { + return "", fmt.Errorf("failed to unmarshal JSON text: %w", err) + } + + doc := csvDocument(value) + if len(doc.metadata) == 0 && len(doc.rows) == 0 { + return "", nil + } + + var buf bytes.Buffer + writeCSVMetadata(&buf, doc.metadata) + if len(doc.rows) == 0 { + return buf.String(), nil + } + + headers := csvHeaders(doc.rows) + if len(headers) == 0 { + return buf.String(), nil + } + + writer := csv.NewWriter(&buf) + if err := writer.Write(headers); err != nil { + return "", fmt.Errorf("failed to write CSV header: %w", err) + } + + for _, row := range doc.rows { + record := make([]string, len(headers)) + for i, header := range headers { + record[i] = row[header] + } + if err := writer.Write(record); err != nil { + return "", fmt.Errorf("failed to write CSV row: %w", err) + } + } + + writer.Flush() + if err := writer.Error(); err != nil { + return "", fmt.Errorf("failed to flush CSV: %w", err) + } + return buf.String(), nil +} + +func csvDocument(value any) csvOutputDocument { + switch v := value.(type) { + case []any: + return csvOutputDocument{rows: csvRowsFromArray(v)} + case map[string]any: + if rows, metadata, ok := primaryRowsFromMap(v); ok { + return csvOutputDocument{ + metadata: newFlattenedCSVRow(metadata), + rows: csvRowsFromArray(rows), + } + } + return csvOutputDocument{rows: []map[string]string{newFlattenedCSVRow(v)}} + default: + return csvOutputDocument{rows: []map[string]string{scalarCSVRow(v)}} + } +} + +func primaryRowsFromMap(value map[string]any) ([]any, map[string]any, bool) { + if rows, path, ok := primaryRowsAtCurrentLevel(value); ok { + return rows, metadataWithoutPath(value, path), true + } + if rows, path, ok := primaryRowsOneLevelDown(value); ok { + return rows, metadataWithoutPath(value, path), true + } + return nil, nil, false +} + +func primaryRowsAtCurrentLevel(value map[string]any) ([]any, []string, bool) { + if key, ok := preferredPrimaryRowKey(value); ok { + rows, _ := value[key].([]any) + return rows, []string{key}, true + } + if key, ok := singleArrayKey(value); ok { + rows, _ := value[key].([]any) + return rows, []string{key}, true + } + return nil, nil, false +} + +func primaryRowsOneLevelDown(value map[string]any) ([]any, []string, bool) { + var matchedRows []any + var matchedPath []string + for key, raw := range value { + child, ok := raw.(map[string]any) + if !ok { + continue + } + rows, path, ok := primaryRowsAtCurrentLevel(child) + if !ok { + continue + } + if matchedPath != nil { + return nil, nil, false + } + matchedRows = rows + matchedPath = append([]string{key}, path...) + } + if matchedPath == nil { + return nil, nil, false + } + return matchedRows, matchedPath, true +} + +func metadataWithoutPath(value map[string]any, path []string) map[string]any { + metadata := make(map[string]any, len(value)) + for key, raw := range value { + if key != path[0] { + metadata[key] = raw + continue + } + + if len(path) == 1 { + continue + } + child, ok := raw.(map[string]any) + if !ok { + continue + } + childMetadata := metadataWithoutPath(child, path[1:]) + if len(childMetadata) > 0 { + metadata[key] = childMetadata + } + } + return metadata +} + +func csvRowsFromArray(values []any) []map[string]string { + if len(values) == 0 { + return nil + } + + rows := make([]map[string]string, 0, len(values)) + for _, value := range values { + var row map[string]string + switch v := value.(type) { + case map[string]any: + row = make(map[string]string) + appendFlattenedCSVFields(row, v, "") + default: + row = scalarCSVRow(v) + } + rows = append(rows, row) + } + return rows +} + +func writeCSVMetadata(buf *bytes.Buffer, metadata map[string]string) { + if len(metadata) == 0 { + return + } + + headers := make([]string, 0, len(metadata)) + for header := range metadata { + headers = append(headers, header) + } + sort.Strings(headers) + + for _, header := range headers { + fmt.Fprintf(buf, "# %s: %s\n", header, normalizeCSVWhitespace(metadata[header])) + } + buf.WriteByte('\n') +} + +func newFlattenedCSVRow(value map[string]any) map[string]string { + row := make(map[string]string) + appendFlattenedCSVFields(row, value, "") + return row +} + +func appendFlattenedCSVFields(row map[string]string, value map[string]any, prefix string) { + if value == nil { + return + } + + for key, raw := range value { + column := csvColumnName(prefix, key) + switch v := raw.(type) { + case map[string]any: + appendFlattenedCSVFields(row, v, column) + case []any: + row[column] = csvArrayValue(v) + default: + row[column] = csvColumnValue(column, v) + } + } +} + +func csvHeaders(rows []map[string]string) []string { + headerSet := make(map[string]struct{}) + for _, row := range rows { + for header := range row { + headerSet[header] = struct{}{} + } + } + + headers := make([]string, 0, len(headerSet)) + for header := range headerSet { + headers = append(headers, header) + } + sort.Strings(headers) + return headers +} + +func csvColumnName(prefix, key string) string { + if prefix == "" { + return key + } + return prefix + "." + key +} + +func preferredPrimaryRowKey(value map[string]any) (string, bool) { + for _, key := range primaryCSVRowKeys { + if _, ok := value[key].([]any); ok { + return key, true + } + } + return "", false +} + +func singleArrayKey(value map[string]any) (string, bool) { + var arrayKey string + for key, raw := range value { + if _, ok := raw.([]any); !ok { + continue + } + if arrayKey != "" { + return "", false + } + arrayKey = key + } + if arrayKey == "" { + return "", false + } + return arrayKey, true +} + +func csvColumnValue(column string, value any) string { + str := scalarCSVValue(value) + if isBodyColumn(column) { + return normalizeCSVWhitespace(str) + } + return str +} + +func csvArrayValue(values []any) string { + if len(values) == 0 { + return "" + } + + // Scalar arrays use semicolons for compactness. This is lossy if an + // element contains a semicolon; use JSON mode when exact reconstruction matters. + parts := make([]string, 0, len(values)) + for _, value := range values { + switch value.(type) { + case map[string]any, []any: + encoded, err := json.Marshal(value) + if err != nil { + parts = append(parts, scalarCSVValue(value)) + } else { + parts = append(parts, string(encoded)) + } + default: + parts = append(parts, scalarCSVValue(value)) + } + } + return strings.Join(parts, ";") +} + +func scalarCSVRow(value any) map[string]string { + return map[string]string{"value": scalarCSVValue(value)} +} + +func scalarCSVValue(value any) string { + switch v := value.(type) { + case nil: + return "" + case string: + return v + case json.Number: + return v.String() + case bool: + if v { + return "true" + } + return "false" + default: + return fmt.Sprint(v) + } +} + +func isBodyColumn(column string) bool { + return column == "body" || strings.HasSuffix(column, ".body") +} + +func normalizeCSVWhitespace(value string) string { + return strings.Join(strings.Fields(value), " ") +} diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go new file mode 100644 index 0000000000..80ff8a7892 --- /dev/null +++ b/pkg/github/csv_output_test.go @@ -0,0 +1,415 @@ +package github + +import ( + "context" + "encoding/csv" + "encoding/json" + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCSVOutputVariantsAreFeatureGated(t *testing.T) { + listTool := testCSVOutputTool("list_things", `[{"number":1}]`) + getTool := testCSVOutputTool("get_thing", `{"number":1}`) + + tools := withCSVOutputVariants([]inventory.ServerTool{listTool, getTool}) + require.Len(t, tools, 3) + + inv := buildCSVOutputInventory(t, tools, false) + available := inv.AvailableTools(context.Background()) + require.Len(t, available, 2) + + jsonOnly := requireToolByName(t, available, "list_things") + assert.Empty(t, jsonOnly.FeatureFlagEnable) + assert.Equal(t, FeatureFlagCSVOutput, jsonOnly.FeatureFlagDisable) + + getThing := requireToolByName(t, available, "get_thing") + assert.Empty(t, getThing.FeatureFlagEnable) + assert.Empty(t, getThing.FeatureFlagDisable) + + inv = buildCSVOutputInventory(t, tools, true) + available = inv.AvailableTools(context.Background()) + require.Len(t, available, 2) + + csvCapable := requireToolByName(t, available, "list_things") + assert.Equal(t, FeatureFlagCSVOutput, csvCapable.FeatureFlagEnable) + assert.Empty(t, csvCapable.FeatureFlagDisable) +} + +func TestCSVOutputVariantsOnlyApplyToDefaultToolsets(t *testing.T) { + nonDefaultListTool := testCSVOutputToolWithToolset("list_discussions", `[{"number":1}]`, ToolsetMetadataDiscussions) + + tools := withCSVOutputVariants([]inventory.ServerTool{nonDefaultListTool}) + require.Len(t, tools, 1) + + assert.Empty(t, tools[0].FeatureFlagEnable) + assert.Empty(t, tools[0].FeatureFlagDisable) +} + +func TestCSVOutputVariantDoesNotExposeFormatParameter(t *testing.T) { + tools := withCSVOutputVariants([]inventory.ServerTool{testCSVOutputTool("list_things", `[{"number":1}]`)}) + csvCapable := requireCSVOutputVariant(t, tools) + + schema, ok := csvCapable.Tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok) + assert.NotContains(t, schema.Properties, "output_format") +} + +func TestCSVOutputVariantConvertsJSONTextToCSV(t *testing.T) { + tools := withCSVOutputVariants([]inventory.ServerTool{ + testCSVOutputTool("list_things", `[ + { + "number": 1, + "body": "first line\n\tsecond line", + "labels": ["bug", "help wanted"], + "user": {"login": "octocat"} + } + ]`), + }) + inv := buildCSVOutputInventory(t, tools, true) + csvCapable := requireToolByName(t, inv.AvailableTools(context.Background()), "list_things") + + result, err := csvCapable.Handler(nil)(context.Background(), testCSVOutputRequest()) + require.NoError(t, err) + require.NotNil(t, result) + require.False(t, result.IsError) + + assert.NotContains(t, textResult(t, result), "#") + + records := readCSVResult(t, result) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "first line second line", row["body"]) + assert.Equal(t, "bug;help wanted", row["labels"]) + assert.Equal(t, "1", row["number"]) + assert.Equal(t, "octocat", row["user.login"]) +} + +func TestCSVOutputVariantMovesMetadataToPreamble(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "issues": [ + {"number": 1, "title": "First issue"} + ], + "pageInfo": { + "endCursor": "cursor-1", + "hasNextPage": true + }, + "totalCount": 2 + }`) + require.NoError(t, err) + assert.Contains(t, csvText, "# pageInfo.endCursor: cursor-1\n") + assert.Contains(t, csvText, "# pageInfo.hasNextPage: true\n") + assert.Contains(t, csvText, "# totalCount: 2\n\n") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "1", row["number"]) + assert.Equal(t, "First issue", row["title"]) + assert.NotContains(t, row, "pageInfo.endCursor") + assert.NotContains(t, row, "totalCount") +} + +func TestJSONOnlyVariantPreservesOriginalJSONText(t *testing.T) { + const jsonResponse = `[{"number":1,"user":{"login":"octocat"}}]` + tools := withCSVOutputVariants([]inventory.ServerTool{testCSVOutputTool("list_things", jsonResponse)}) + inv := buildCSVOutputInventory(t, tools, false) + jsonOnly := requireToolByName(t, inv.AvailableTools(context.Background()), "list_things") + + result, err := jsonOnly.Handler(nil)(context.Background(), testCSVOutputRequest()) + require.NoError(t, err) + require.NotNil(t, result) + + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.JSONEq(t, jsonResponse, text.Text) +} + +func TestJSONTextToCSVFlattensPrimaryRows(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "discussions": [ + { + "number": 5, + "title": "Discussion tools testing", + "category": {"name": "Q&A"}, + "user": {"login": "octocat"} + } + ] + }`) + require.NoError(t, err) + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "Q&A", row["category.name"]) + assert.Equal(t, "5", row["number"]) + assert.Equal(t, "Discussion tools testing", row["title"]) + assert.Equal(t, "octocat", row["user.login"]) +} + +func TestJSONTextToCSVFindsPrimaryRowsOneLevelDeeper(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "issues": { + "nodes": [ + {"number": 5, "title": "Nested issue"} + ], + "pageInfo": {"hasNextPage": false}, + "totalCount": 1 + } + }`) + require.NoError(t, err) + + assert.Contains(t, csvText, "# issues.pageInfo.hasNextPage: false\n") + assert.Contains(t, csvText, "# issues.totalCount: 1\n\n") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "5", row["number"]) + assert.Equal(t, "Nested issue", row["title"]) +} + +func TestJSONTextToCSVUsesSingleArrayAsPrimaryRows(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "results": [ + {"number": 1, "title": "Single array result"} + ], + "pageInfo": {"hasNextPage": true} + }`) + require.NoError(t, err) + + assert.Contains(t, csvText, "# pageInfo.hasNextPage: true\n\n") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "1", row["number"]) + assert.Equal(t, "Single array result", row["title"]) + assert.NotContains(t, row, "pageInfo.hasNextPage") +} + +func TestJSONTextToCSVFlattensRootObjectWithoutPrimaryRows(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "name": "summary", + "pageInfo": {"hasNextPage": false}, + "totalCount": 2 + }`) + require.NoError(t, err) + assert.NotContains(t, csvText, "#") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "summary", row["name"]) + assert.Equal(t, "false", row["pageInfo.hasNextPage"]) + assert.Equal(t, "2", row["totalCount"]) +} + +func TestJSONTextToCSVConvertsScalarToValueRow(t *testing.T) { + csvText, err := jsonTextToCSV(`"plain value"`) + require.NoError(t, err) + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "plain value", row["value"]) +} + +func TestJSONTextToCSVReturnsEmptyForEmptyArray(t *testing.T) { + csvText, err := jsonTextToCSV(`[]`) + require.NoError(t, err) + assert.Empty(t, csvText) +} + +func TestJSONTextToCSVReturnsEmptyForEmptyObject(t *testing.T) { + csvText, err := jsonTextToCSV(`{}`) + require.NoError(t, err) + assert.Empty(t, csvText) +} + +func TestJSONTextToCSVReturnsEmptyForOnlyEmptyNestedObjects(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "repository": { + "owner": {} + } + }`) + require.NoError(t, err) + assert.Empty(t, csvText) +} + +func TestJSONTextToCSVReturnsMetadataOnlyWhenRowsHaveNoColumns(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "items": [ + {} + ], + "totalCount": 1 + }`) + require.NoError(t, err) + assert.Equal(t, "# totalCount: 1\n\n", csvText) +} + +func TestJSONTextToCSVFlattensAmbiguousArraysAsSingleRow(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "foo": ["a", "b"], + "bar": ["c"] + }`) + require.NoError(t, err) + assert.NotContains(t, csvText, "#") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "c", row["bar"]) + assert.Equal(t, "a;b", row["foo"]) +} + +func TestJSONTextToCSVUsesPreferredArrayWhenMultipleArraysExist(t *testing.T) { + csvText, err := jsonTextToCSV(`{ + "items": [ + {"id": 1} + ], + "other": [ + {"id": 2} + ], + "totalCount": 1 + }`) + require.NoError(t, err) + + assert.Contains(t, csvText, "# other: {\"id\":2}\n") + assert.Contains(t, csvText, "# totalCount: 1\n\n") + + records := readCSVText(t, csvText) + require.Len(t, records, 2) + + row := csvRow(t, records[0], records[1]) + assert.Equal(t, "1", row["id"]) +} + +func testCSVOutputTool(name string, response string) inventory.ServerTool { + return testCSVOutputToolWithToolset(name, response, ToolsetMetadataRepos) +} + +func testCSVOutputToolWithToolset(name string, response string, toolset inventory.ToolsetMetadata) inventory.ServerTool { + return inventory.ServerTool{ + Tool: mcp.Tool{ + Name: name, + Annotations: &mcp.ToolAnnotations{ + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{}, + }, + }, + Toolset: toolset, + HandlerFunc: func(_ any) mcp.ToolHandler { + return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: response}, + }, + }, nil + } + }, + } +} + +func buildCSVOutputInventory(t *testing.T, tools []inventory.ServerTool, csvOutputEnabled bool) *inventory.Inventory { + t.Helper() + + inv, err := inventory.NewBuilder(). + SetTools(tools). + WithFeatureChecker(func(_ context.Context, flagName string) (bool, error) { + return flagName == FeatureFlagCSVOutput && csvOutputEnabled, nil + }). + Build() + require.NoError(t, err) + return inv +} + +func requireToolByName(t *testing.T, tools []inventory.ServerTool, name string) inventory.ServerTool { + t.Helper() + + for _, tool := range tools { + if tool.Tool.Name == name { + return tool + } + } + require.Failf(t, "tool not found", "tool %q not found", name) + return inventory.ServerTool{} +} + +func requireCSVOutputVariant(t *testing.T, tools []inventory.ServerTool) inventory.ServerTool { + t.Helper() + + for _, tool := range tools { + if tool.FeatureFlagEnable == FeatureFlagCSVOutput { + return tool + } + } + require.Fail(t, "CSV output variant not found") + return inventory.ServerTool{} +} + +func testCSVOutputRequest() *mcp.CallToolRequest { + return &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Arguments: json.RawMessage(`{}`), + }, + } +} + +func readCSVResult(t *testing.T, result *mcp.CallToolResult) [][]string { + t.Helper() + + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + + return readCSVText(t, text.Text) +} + +func textResult(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + return text.Text +} + +func readCSVText(t *testing.T, text string) [][]string { + t.Helper() + + reader := csv.NewReader(strings.NewReader(text)) + reader.Comment = '#' + records, err := reader.ReadAll() + require.NoError(t, err) + return records +} + +func csvRow(t *testing.T, headers []string, record []string) map[string]string { + t.Helper() + require.Len(t, record, len(headers)) + + row := make(map[string]string, len(headers)) + for i, header := range headers { + row[header] = record[i] + } + return row +} diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index eb856e0bd6..e3a031f999 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -410,7 +410,6 @@ func (d *RequestDeps) GetT() translations.TranslationHelperFunc { return d.T } func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags { return FeatureFlags{ LockdownMode: d.lockdownMode && ghcontext.IsLockdownMode(ctx), - InsidersMode: ghcontext.IsInsidersMode(ctx), } } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 3f3d7bf976..9adfa38d2a 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -1,14 +1,23 @@ package github +import "slices" + // MCPAppsFeatureFlag is the feature flag name for MCP Apps (interactive UI forms). const MCPAppsFeatureFlag = "remote_mcp_ui_apps" +// FeatureFlagCSVOutput is the feature flag name for CSV output on list tools. +const FeatureFlagCSVOutput = "csv_output" + +// FeatureFlagIFCLabels is the feature flag name for IFC security labels in tool results. +const FeatureFlagIFCLabels = "ifc_labels" + // 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. // This is the single source of truth for which flags are user-controllable. var AllowedFeatureFlags = []string{ MCPAppsFeatureFlag, + FeatureFlagCSVOutput, FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular, } @@ -19,37 +28,30 @@ var AllowedFeatureFlags = []string{ // feature flag expansion. var InsidersFeatureFlags = []string{ MCPAppsFeatureFlag, + FeatureFlagCSVOutput, + FeatureFlagIFCLabels, } // FeatureFlags defines runtime feature toggles that adjust tool behavior. type FeatureFlags struct { LockdownMode bool - InsidersMode bool } // ResolveFeatureFlags computes the effective set of enabled feature flags by: -// 1. Taking explicitly enabled features (from CLI flags or HTTP headers) -// 2. Adding insiders-expanded features when insiders mode is active -// 3. Validating all features against the AllowedFeatureFlags allowlist +// 1. Taking explicitly enabled features validated against AllowedFeatureFlags +// 2. Adding features enabled by insiders mode from InsidersFeatureFlags // // Returns a set (map) for O(1) lookup by the feature checker. func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool { - allowed := make(map[string]bool, len(AllowedFeatureFlags)) - for _, f := range AllowedFeatureFlags { - allowed[f] = true - } - effective := make(map[string]bool) for _, f := range enabledFeatures { - if allowed[f] { + if slices.Contains(AllowedFeatureFlags, f) { effective[f] = true } } if insidersMode { for _, f := range InsidersFeatureFlags { - if allowed[f] { - effective[f] = true - } + effective[f] = true } } return effective diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index b0c1a4305c..9bc1be473f 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -18,10 +18,14 @@ import ( // RemoteMCPEnthusiasticGreeting is a dummy test feature flag . const RemoteMCPEnthusiasticGreeting = "remote_mcp_enthusiastic_greeting" -// FeatureChecker is an interface for checking if a feature flag is enabled. -type FeatureChecker interface { - // IsFeatureEnabled checks if a feature flag is enabled. - IsFeatureEnabled(ctx context.Context, flagName string) bool +func featureCheckerFor(enabledFlags ...string) func(context.Context, string) (bool, error) { + enabled := make(map[string]bool, len(enabledFlags)) + for _, flag := range enabledFlags { + enabled[flag] = true + } + return func(_ context.Context, flagName string) (bool, error) { + return enabled[flagName], nil + } } // HelloWorld returns a simple greeting tool that demonstrates feature flag conditional behavior. @@ -45,9 +49,6 @@ func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool { if deps.IsFeatureEnabled(ctx, RemoteMCPEnthusiasticGreeting) { greeting += " Welcome to the future of MCP! 🎉" } - if deps.GetFlags(ctx).InsidersMode { - greeting += " Experimental features are enabled! 🚀" - } // Build response response := map[string]any{ @@ -89,12 +90,9 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - // Create feature checker based on test case - checker := func(_ context.Context, flagName string) (bool, error) { - if flagName == RemoteMCPEnthusiasticGreeting { - return tt.featureFlagEnabled, nil - } - return false, nil + var enabledFlags []string + if tt.featureFlagEnabled { + enabledFlags = append(enabledFlags, RemoteMCPEnthusiasticGreeting) } // Create deps with the checker @@ -103,7 +101,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { translations.NullTranslationHelper, FeatureFlags{}, 0, - checker, + featureCheckerFor(enabledFlags...), stubExporters(), ) @@ -149,14 +147,12 @@ func TestResolveFeatureFlags(t *testing.T) { { name: "no features, no insiders", enabledFeatures: nil, - insidersMode: false, expectedFlags: nil, - unexpectedFlags: []string{MCPAppsFeatureFlag}, + unexpectedFlags: []string{MCPAppsFeatureFlag, FeatureFlagIFCLabels}, }, { name: "explicit feature enabled", enabledFeatures: []string{MCPAppsFeatureFlag}, - insidersMode: false, expectedFlags: []string{MCPAppsFeatureFlag}, }, { @@ -165,16 +161,26 @@ func TestResolveFeatureFlags(t *testing.T) { insidersMode: true, expectedFlags: InsidersFeatureFlags, }, + { + name: "insiders mode enables internal-only flags", + enabledFeatures: nil, + insidersMode: true, + expectedFlags: []string{FeatureFlagIFCLabels}, + }, + { + name: "internal-only flags are not directly enabled", + enabledFeatures: []string{FeatureFlagIFCLabels}, + expectedFlags: nil, + unexpectedFlags: []string{FeatureFlagIFCLabels}, + }, { name: "unknown flags are filtered out", enabledFeatures: []string{"unknown_flag", "another_unknown"}, - insidersMode: false, unexpectedFlags: []string{"unknown_flag", "another_unknown"}, }, { name: "mix of known and unknown flags", enabledFeatures: []string{MCPAppsFeatureFlag, "unknown_flag"}, - insidersMode: false, expectedFlags: []string{MCPAppsFeatureFlag}, unexpectedFlags: []string{"unknown_flag"}, }, @@ -182,7 +188,7 @@ func TestResolveFeatureFlags(t *testing.T) { name: "explicit plus insiders deduplicates", enabledFeatures: []string{MCPAppsFeatureFlag}, insidersMode: true, - expectedFlags: []string{MCPAppsFeatureFlag}, + expectedFlags: InsidersFeatureFlags, }, } @@ -199,66 +205,3 @@ func TestResolveFeatureFlags(t *testing.T) { }) } } - -func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - insidersMode bool - expectedGreeting string - }{ - { - name: "Experimental disabled - default greeting", - insidersMode: false, - expectedGreeting: "Hello, world!", - }, - { - name: "Experimental enabled - experimental greeting", - insidersMode: true, - expectedGreeting: "Hello, world! Experimental features are enabled! 🚀", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - // Create deps with the checker - deps := NewBaseDeps( - nil, nil, nil, nil, - translations.NullTranslationHelper, - FeatureFlags{InsidersMode: tt.insidersMode}, - 0, - nil, - stubExporters(), - ) - - // Get the tool and its handler - tool := HelloWorldTool(translations.NullTranslationHelper) - handler := tool.Handler(deps) - - // Call the handler with deps in context - ctx := ContextWithDeps(context.Background(), deps) - result, err := handler(ctx, &mcp.CallToolRequest{ - Params: &mcp.CallToolParamsRaw{ - Arguments: json.RawMessage(`{}`), - }, - }) - require.NoError(t, err) - require.NotNil(t, result) - require.Len(t, result.Content, 1) - - // Parse the response - should be TextContent - textContent, ok := result.Content[0].(*mcp.TextContent) - require.True(t, ok, "expected content to be TextContent") - - var response map[string]any - err = json.Unmarshal([]byte(textContent.Text), &response) - require.NoError(t, err) - - // Verify the greeting matches expected based on feature flag - assert.Equal(t, tt.expectedGreeting, response["greeting"]) - }) - } -} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index fe1e7b5011..0bb9ce8f30 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1400,12 +1400,12 @@ Options are: return utils.NewToolResultError(err.Error()), nil, nil } - // When insiders mode is enabled and the client supports MCP Apps UI, + // When MCP Apps are enabled and the client supports UI, // check if this is a UI form submission. The UI sends _ui_submitted=true // to distinguish form submissions from LLM calls. uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") - if deps.GetFlags(ctx).InsidersMode && clientSupportsUI(ctx, req) && !uiSubmitted { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted { if method == "update" { // Skip the UI form when a state change is requested because // the form only handles title/body editing and would lose the diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index ff4cb93a16..6e020dea09 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1266,9 +1266,9 @@ func Test_CreateIssue(t *testing.T) { } } -// Test_IssueWrite_InsidersMode_UIGate verifies the insiders mode UI gate +// Test_IssueWrite_MCPAppsFeature_UIGate verifies the MCP Apps feature UI gate // behavior: UI clients get a form message, non-UI clients execute directly. -func Test_IssueWrite_InsidersMode_UIGate(t *testing.T) { +func Test_IssueWrite_MCPAppsFeature_UIGate(t *testing.T) { t.Parallel() mockIssue := &github.Issue{ @@ -1284,9 +1284,9 @@ func Test_IssueWrite_InsidersMode_UIGate(t *testing.T) { })) deps := BaseDeps{ - Client: client, - GQLClient: githubv4.NewClient(nil), - Flags: FeatureFlags{InsidersMode: true}, + Client: client, + GQLClient: githubv4.NewClient(nil), + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), } handler := serverTool.Handler(deps) @@ -1401,9 +1401,9 @@ func Test_IssueWrite_InsidersMode_UIGate(t *testing.T) { )) closeDeps := BaseDeps{ - Client: closeClient, - GQLClient: closeGQLClient, - Flags: FeatureFlags{InsidersMode: true}, + Client: closeClient, + GQLClient: closeGQLClient, + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), } closeHandler := serverTool.Handler(closeDeps) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 9672f85244..7e2391455f 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -611,12 +611,12 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultError(err.Error()), nil, nil } - // When insiders mode is enabled and the client supports MCP Apps UI, + // When MCP Apps are enabled and the client supports UI, // check if this is a UI form submission. The UI sends _ui_submitted=true // to distinguish form submissions from LLM calls. uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") - if deps.GetFlags(ctx).InsidersMode && clientSupportsUI(ctx, req) && !uiSubmitted { + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted { return utils.NewToolResultText(fmt.Sprintf("Ready to create a pull request in %s/%s. IMPORTANT: The PR has NOT been created yet. Do NOT tell the user the PR was created. The user MUST click Submit in the form to create it.", owner, repo)), nil, nil } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index a73ba2e173..097651b66e 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2393,9 +2393,9 @@ func Test_CreatePullRequest(t *testing.T) { } } -// Test_CreatePullRequest_InsidersMode_UIGate verifies the insiders mode UI gate +// Test_CreatePullRequest_MCPAppsFeature_UIGate verifies the MCP Apps feature UI gate // behavior: UI clients get a form message, non-UI clients execute directly. -func Test_CreatePullRequest_InsidersMode_UIGate(t *testing.T) { +func Test_CreatePullRequest_MCPAppsFeature_UIGate(t *testing.T) { t.Parallel() mockPR := &github.PullRequest{ @@ -2414,9 +2414,9 @@ func Test_CreatePullRequest_InsidersMode_UIGate(t *testing.T) { })) deps := BaseDeps{ - Client: client, - GQLClient: githubv4.NewClient(nil), - Flags: FeatureFlags{InsidersMode: true}, + Client: client, + GQLClient: githubv4.NewClient(nil), + featureChecker: featureCheckerFor(MCPAppsFeatureFlag), } handler := serverTool.Handler(deps) diff --git a/pkg/github/server.go b/pkg/github/server.go index 41e502db3c..9df7c59b6c 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -50,7 +50,7 @@ type MCPServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool - // InsidersMode indicates if we should enable experimental features + // InsidersMode expands to the curated set of feature flags enabled for insiders. InsidersMode bool // Logger is used for logging within the server diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index be37ca949d..7f909f431c 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -130,7 +130,6 @@ func mockRESTPermissionServer(t *testing.T, defaultPerm string, overrides map[st func stubFeatureFlags(enabledFlags map[string]bool) FeatureFlags { return FeatureFlags{ LockdownMode: enabledFlags["lockdown-mode"], - InsidersMode: enabledFlags["insiders-mode"], } } @@ -164,7 +163,6 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { Translator: translations.NullTranslationHelper, ContentWindowSize: 5000, LockdownMode: false, - InsidersMode: false, } deps := stubDeps{obsv: stubExporters()} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 7d22c72fc9..e7530b672d 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -167,7 +167,7 @@ var ( // AllTools returns all tools with their embedded toolset metadata. // Tool functions return ServerTool directly with toolset info. func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { - return []inventory.ServerTool{ + return withCSVOutputVariants([]inventory.ServerTool{ // Context tools GetMe(t), GetTeams(t), @@ -313,7 +313,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GranularAddPullRequestReviewComment(t), GranularResolveReviewThread(t), GranularUnresolveReviewThread(t), - } + }) } // ToBoolPtr converts a bool to a *bool pointer. diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index 90e3c744cb..0a4a4eb7b0 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -1,6 +1,11 @@ package github import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "strings" "testing" "github.com/github/github-mcp-server/pkg/inventory" @@ -184,3 +189,29 @@ func TestToolsetMetadataConsistency(t *testing.T) { } } } + +func TestGitHubPackageDoesNotReadInsidersMode(t *testing.T) { + files, err := filepath.Glob("*.go") + require.NoError(t, err) + + for _, file := range files { + if strings.HasSuffix(file, "_test.go") { + continue + } + + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, file, nil, 0) + require.NoError(t, err, "failed to parse %s", file) + + ast.Inspect(node, func(n ast.Node) bool { + selector, ok := n.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != "InsidersMode" { + return true + } + + position := fset.Position(selector.Sel.Pos()) + t.Errorf("%s reads InsidersMode directly; gate behavior on concrete feature flags instead", position) + return true + }) + } +} diff --git a/pkg/github/ui_embed.go b/pkg/github/ui_embed.go index 257856e156..c3f1cef9d2 100644 --- a/pkg/github/ui_embed.go +++ b/pkg/github/ui_embed.go @@ -34,7 +34,7 @@ func MustGetUIAsset(name string) string { // UIAssetsAvailable returns true if the MCP App UI assets have been built. // This checks for a known UI asset file to determine if `script/build-ui` has been run. // Use this to gracefully skip UI registration when assets aren't available, -// allowing Insiders mode to work for non-UI features without requiring a UI build. +// allowing non-UI features to work without requiring a UI build. func UIAssetsAvailable() bool { _, err := GetUIAsset("get-me.html") return err == nil diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 90423d93cc..dd6161d52f 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -321,8 +321,7 @@ func hasStaticConfig(cfg *ServerConfig) bool { return cfg.ReadOnly || cfg.EnabledToolsets != nil || cfg.EnabledTools != nil || - len(cfg.ExcludeTools) > 0 || - cfg.InsidersMode + len(cfg.ExcludeTools) > 0 } // buildStaticInventory pre-filters the full tool/resource/prompt universe using @@ -353,8 +352,12 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) } + // Static filtering defines an upper bound for all requests. Do not apply + // feature flags here: HTTP feature flags can come from request context + // (/insiders, X-MCP-Features), so variants must be preserved until the + // per-request inventory evaluates them. ctx := context.Background() - return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) + return inv.AvailableToolsWithoutFeatureFiltering(ctx), inv.AvailableResourceTemplatesWithoutFeatureFiltering(ctx), inv.AvailablePromptsWithoutFeatureFiltering(ctx) } // InventoryFiltersForRequest applies filters to the inventory builder diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index fd2966fd05..dfb402093b 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -632,6 +632,31 @@ func TestStaticConfigEnforcement(t *testing.T) { } } +func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { + tools := []inventory.ServerTool{ + mockToolWithFeatureFlag("list_issues", "issues", true, "", github.FeatureFlagCSVOutput), + mockToolWithFeatureFlag("list_issues", "issues", true, github.FeatureFlagCSVOutput, ""), + } + cfg := &ServerConfig{Version: "test", EnabledToolsets: []string{"issues"}} + featureChecker := createHTTPFeatureChecker(nil, false) + + staticTools, _, _ := buildStaticInventoryFromTools(cfg, tools, featureChecker) + require.Len(t, staticTools, 2, "static upper bounds should preserve both feature variants") + + inv, err := inventory.NewBuilder(). + SetTools(staticTools). + WithFeatureChecker(featureChecker). + WithToolsets([]string{"all"}). + Build() + require.NoError(t, err) + + ctx := ghcontext.WithInsidersMode(context.Background(), true) + available := inv.AvailableTools(ctx) + require.Len(t, available, 1) + assert.Equal(t, "list_issues", available[0].Tool.Name) + assert.Equal(t, github.FeatureFlagCSVOutput, available[0].FeatureFlagEnable) +} + // TestContentTypeHandling verifies that the MCP StreamableHTTP handler // accepts Content-Type values with additional parameters like charset=utf-8. // This is a regression test for https://github.com/github/github-mcp-server/issues/2333 @@ -754,7 +779,7 @@ func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTo } ctx := context.Background() - return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) + return inv.AvailableToolsWithoutFeatureFiltering(ctx), inv.AvailableResourceTemplatesWithoutFeatureFiltering(ctx), inv.AvailablePromptsWithoutFeatureFiltering(ctx) } func TestCrossOriginProtection(t *testing.T) { @@ -847,7 +872,7 @@ func TestInsidersRoutePreservesUIMeta(t *testing.T) { uiTool := mockTool("with_ui", "repos", true) uiTool.Tool.Meta = mcp.Meta{"ui": map[string]any{"resourceUri": uiURI}} - checker := createHTTPFeatureChecker() + checker := createHTTPFeatureChecker(nil, false) build := func() *inventory.Inventory { inv, err := inventory.NewBuilder(). SetTools([]inventory.ServerTool{uiTool}). diff --git a/pkg/http/middleware/cors.go b/pkg/http/middleware/cors.go index 2eaf4227b4..fb8e8e548d 100644 --- a/pkg/http/middleware/cors.go +++ b/pkg/http/middleware/cors.go @@ -17,6 +17,7 @@ func SetCorsHeaders(h http.Handler) http.Handler { "Mcp-Session-Id", "Mcp-Protocol-Version", "Last-Event-ID", + "X-Custom-Auth-Headers", headers.AuthorizationHeader, headers.MCPReadOnlyHeader, headers.MCPToolsetsHeader, diff --git a/pkg/http/server.go b/pkg/http/server.go index b8c419ea04..6fd19a8b9b 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -82,7 +82,10 @@ type ServerConfig struct { // When set via CLI flag, per-request headers cannot re-include these tools. ExcludeTools []string - // InsidersMode indicates if we should enable experimental features. + // EnabledFeatures is a list of feature flags that are enabled. + EnabledFeatures []string + + // InsidersMode expands to the curated set of feature flags enabled for insiders. InsidersMode bool } @@ -121,7 +124,7 @@ func RunHTTPServer(cfg ServerConfig) error { repoAccessOpts = append(repoAccessOpts, lockdown.WithTTL(*cfg.RepoAccessCacheTTL)) } - featureChecker := createHTTPFeatureChecker() + featureChecker := createHTTPFeatureChecker(cfg.EnabledFeatures, cfg.InsidersMode) obs, err := observability.NewExporters(logger, metrics.NewNoopMetrics()) if err != nil { @@ -228,14 +231,16 @@ func initGlobalToolScopeMap(t translations.TranslationHelperFunc) error { return nil } -// createHTTPFeatureChecker creates a feature checker that resolves features -// per-request by reading header features and insiders mode from context, -// then validating against the centralized AllowedFeatureFlags allowlist. -func createHTTPFeatureChecker() inventory.FeatureFlagChecker { +// createHTTPFeatureChecker creates a feature checker that resolves static CLI +// features plus per-request header features and insiders mode. +func createHTTPFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { return func(ctx context.Context, flag string) (bool, error) { headerFeatures := ghcontext.GetHeaderFeatures(ctx) - insidersMode := ghcontext.IsInsidersMode(ctx) - effective := github.ResolveFeatureFlags(headerFeatures, insidersMode) + features := make([]string, 0, len(enabledFeatures)+len(headerFeatures)) + features = append(features, enabledFeatures...) + features = append(features, headerFeatures...) + + effective := github.ResolveFeatureFlags(features, insidersMode || ghcontext.IsInsidersMode(ctx)) return effective[flag], nil } } diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index 23c82d0486..5458a6b395 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -11,10 +11,10 @@ import ( ) func TestCreateHTTPFeatureChecker(t *testing.T) { - checker := createHTTPFeatureChecker() - tests := []struct { name string + staticFeatures []string + staticInsiders bool flagName string headerFeatures []string insidersMode bool @@ -74,6 +74,37 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { insidersMode: true, wantEnabled: true, }, + { + name: "static feature is enabled without header", + staticFeatures: []string{github.FeatureFlagCSVOutput}, + flagName: github.FeatureFlagCSVOutput, + wantEnabled: true, + }, + { + name: "static features combine with header features", + staticFeatures: []string{github.FeatureFlagCSVOutput}, + flagName: github.FeatureFlagIssuesGranular, + headerFeatures: []string{github.FeatureFlagIssuesGranular}, + wantEnabled: true, + }, + { + name: "internal-only flag in header is ignored", + flagName: github.FeatureFlagIFCLabels, + headerFeatures: []string{github.FeatureFlagIFCLabels}, + wantEnabled: false, + }, + { + name: "static insiders enables insiders flags without route context", + staticInsiders: true, + flagName: github.FeatureFlagCSVOutput, + wantEnabled: true, + }, + { + name: "insiders mode enables internal-only insiders flags", + flagName: github.FeatureFlagIFCLabels, + insidersMode: true, + wantEnabled: true, + }, { name: "insiders mode does not enable granular flags", flagName: github.FeatureFlagIssuesGranular, @@ -84,6 +115,7 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + checker := createHTTPFeatureChecker(tt.staticFeatures, tt.staticInsiders) ctx := context.Background() if len(tt.headerFeatures) > 0 { ctx = ghcontext.WithHeaderFeatures(ctx, tt.headerFeatures) diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 604aa1000d..20a157de63 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -58,6 +58,10 @@ func (r *Inventory) isFeatureFlagAllowed(ctx context.Context, enableFlag, disabl // 4. Builder filters (via WithFilter) // 5. Toolset/additional tools func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { + return r.isToolEnabledWithFeatureFlags(ctx, tool, true) +} + +func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *ServerTool, checkFeatureFlags bool) bool { // 1. Check tool's own Enabled function first if tool.Enabled != nil { enabled, err := tool.Enabled(ctx) @@ -70,7 +74,7 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { } } // 2. Check feature flags - if !r.isFeatureFlagAllowed(ctx, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { + if checkFeatureFlags && !r.isFeatureFlagAllowed(ctx, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { return false } // 3. Check read-only filter (applies to all tools) @@ -99,6 +103,15 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { return true } +func sortTools(tools []ServerTool) { + sort.Slice(tools, func(i, j int) bool { + if tools[i].Toolset.ID != tools[j].Toolset.ID { + return tools[i].Toolset.ID < tools[j].Toolset.ID + } + return tools[i].Tool.Name < tools[j].Tool.Name + }) +} + // AvailableTools returns the tools that pass all current filters, // sorted deterministically by toolset ID, then tool name. // The context is used for feature flag evaluation. @@ -112,16 +125,36 @@ func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool { } // Sort deterministically: by toolset ID, then by tool name - sort.Slice(result, func(i, j int) bool { - if result[i].Toolset.ID != result[j].Toolset.ID { - return result[i].Toolset.ID < result[j].Toolset.ID + sortTools(result) + + return result +} + +// AvailableToolsWithoutFeatureFiltering returns tools that pass every filter +// except FeatureFlagEnable/FeatureFlagDisable. +func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) []ServerTool { + var result []ServerTool + for i := range r.tools { + tool := &r.tools[i] + if r.isToolEnabledWithFeatureFlags(ctx, tool, false) { + result = append(result, *tool) } - return result[i].Tool.Name < result[j].Tool.Name - }) + } + + sortTools(result) return result } +func sortResourceTemplates(resourceTemplates []ServerResourceTemplate) { + sort.Slice(resourceTemplates, func(i, j int) bool { + if resourceTemplates[i].Toolset.ID != resourceTemplates[j].Toolset.ID { + return resourceTemplates[i].Toolset.ID < resourceTemplates[j].Toolset.ID + } + return resourceTemplates[i].Template.Name < resourceTemplates[j].Template.Name + }) +} + // AvailableResourceTemplates returns resource templates that pass all current filters, // sorted deterministically by toolset ID, then template name. // The context is used for feature flag evaluation. @@ -139,16 +172,36 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso } // Sort deterministically: by toolset ID, then by template name - sort.Slice(result, func(i, j int) bool { - if result[i].Toolset.ID != result[j].Toolset.ID { - return result[i].Toolset.ID < result[j].Toolset.ID + sortResourceTemplates(result) + + return result +} + +// AvailableResourceTemplatesWithoutFeatureFiltering returns resource templates +// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. +func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.Context) []ServerResourceTemplate { + var result []ServerResourceTemplate + for i := range r.resourceTemplates { + res := &r.resourceTemplates[i] + if r.isToolsetEnabled(res.Toolset.ID) { + result = append(result, *res) } - return result[i].Template.Name < result[j].Template.Name - }) + } + + sortResourceTemplates(result) return result } +func sortPrompts(prompts []ServerPrompt) { + sort.Slice(prompts, func(i, j int) bool { + if prompts[i].Toolset.ID != prompts[j].Toolset.ID { + return prompts[i].Toolset.ID < prompts[j].Toolset.ID + } + return prompts[i].Prompt.Name < prompts[j].Prompt.Name + }) +} + // AvailablePrompts returns prompts that pass all current filters, // sorted deterministically by toolset ID, then prompt name. // The context is used for feature flag evaluation. @@ -166,12 +219,23 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { } // Sort deterministically: by toolset ID, then by prompt name - sort.Slice(result, func(i, j int) bool { - if result[i].Toolset.ID != result[j].Toolset.ID { - return result[i].Toolset.ID < result[j].Toolset.ID + sortPrompts(result) + + return result +} + +// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every filter +// except FeatureFlagEnable/FeatureFlagDisable. +func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt { + var result []ServerPrompt + for i := range r.prompts { + prompt := &r.prompts[i] + if r.isToolsetEnabled(prompt.Toolset.ID) { + result = append(result, *prompt) } - return result[i].Prompt.Name < result[j].Prompt.Name - }) + } + + sortPrompts(result) return result } From bfa6fee63bced0c82f4ceb2e7e237d8b672fa89b Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Thu, 21 May 2026 09:23:56 +0100 Subject: [PATCH 2/8] fix: resolve rebase feature flag conflicts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 8 ++++---- pkg/github/issues_test.go | 35 +++++++++++++++------------------ pkg/github/repositories.go | 4 ++-- pkg/github/repositories_test.go | 13 ++++++------ pkg/github/search.go | 2 +- pkg/github/search_test.go | 9 ++++----- 6 files changed, 33 insertions(+), 38 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0bb9ce8f30..6389947df4 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -348,10 +348,10 @@ Options are: } // attachIFC adds the IFC label to a successful tool result when - // InsidersMode is enabled. If the visibility lookup fails the + // IFC labels are enabled. If the visibility lookup fails the // label is omitted rather than misclassifying the result. attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult { - if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode { + if r == nil || r.IsError || !deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { return r } isPrivate, err := FetchRepoIsPrivate(ctx, client, owner, repo) @@ -1044,7 +1044,7 @@ 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) { var options []searchOption - if deps.GetFlags(ctx).InsidersMode { + if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { options = append(options, withSearchPostProcess(searchIssuesIFCPostProcess(deps))) } result, err := searchIssuesHandler(ctx, deps, args, options...) @@ -1900,7 +1900,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } result := MarshalledTextResult(resp) - if deps.GetFlags(ctx).InsidersMode { + if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { if result.Meta == nil { result.Meta = mcp.Meta{} } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 6e020dea09..18bab11c1b 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -325,7 +325,6 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode disabled omits ifc label", func(t *testing.T) { deps := BaseDeps{ Client: mustNewGHClient(t, makeMockClient(false, 0)), - Flags: FeatureFlags{InsidersMode: false}, } handler := serverTool.Handler(deps) @@ -339,8 +338,8 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode enabled on public repo emits public untrusted", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(false, 0)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(false, 0)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -357,8 +356,8 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode enabled on private repo with get_comments emits private untrusted", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(true, 0)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(true, 0)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -375,8 +374,8 @@ func Test_IssueRead_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode skips ifc label when visibility lookup fails", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(false, http.StatusInternalServerError)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(false, http.StatusInternalServerError)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -910,7 +909,6 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "public-repo", 1)}} deps := BaseDeps{ Client: mustNewGHClient(t, makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})), - Flags: FeatureFlags{InsidersMode: false}, } handler := serverTool.Handler(deps) @@ -924,8 +922,8 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode all public emits public untrusted", func(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{makeIssue("octocat", "public-repo", 1)}} deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(searchResult, []repoFixture{{owner: "octocat", repo: "public-repo"}})), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -950,7 +948,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { {owner: "octocat", repo: "private-repo", isPrivate: true}, {owner: "octocat", repo: "public-repo"}, })), - Flags: FeatureFlags{InsidersMode: true}, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -971,7 +969,7 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { Client: mustNewGHClient(t, makeMockClient(searchResult, []repoFixture{ {owner: "octocat", repo: "broken", repoStatus: http.StatusInternalServerError}, })), - Flags: FeatureFlags{InsidersMode: true}, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -989,8 +987,8 @@ func Test_SearchIssues_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode empty results emits public untrusted", func(t *testing.T) { searchResult := &github.IssuesSearchResult{Issues: []*github.Issue{}} deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(searchResult, nil)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(searchResult, nil)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -1876,7 +1874,6 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) deps := BaseDeps{ GQLClient: gqlClient, - Flags: FeatureFlags{InsidersMode: false}, } handler := serverTool.Handler(deps) @@ -1892,8 +1889,8 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(false)) gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) deps := BaseDeps{ - GQLClient: gqlClient, - Flags: FeatureFlags{InsidersMode: true}, + GQLClient: gqlClient, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -1919,8 +1916,8 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { matcher := githubv4mock.NewQueryMatcher(query, vars, makeResponse(true)) gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) deps := BaseDeps{ - GQLClient: gqlClient, - Flags: FeatureFlags{InsidersMode: true}, + GQLClient: gqlClient, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 2ca1cf3a7a..d682b5c3d7 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -741,7 +741,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool } // attachIFC adds the IFC label to a successful tool result when - // InsidersMode is enabled. The visibility lookup is performed + // IFC labels are enabled. The visibility lookup is performed // lazily on first use and cached because GetFileContents has // many possible return paths and would otherwise re-fetch on // each. If the visibility lookup fails we skip the label rather @@ -752,7 +752,7 @@ func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool ifcIsPrivate bool ) attachIFC := func(r *mcp.CallToolResult) *mcp.CallToolResult { - if r == nil || r.IsError || !deps.GetFlags(ctx).InsidersMode { + if r == nil || r.IsError || !deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { return r } if !ifcLabelKnown { diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index a44bad65b6..03535f1d26 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -521,7 +521,6 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode disabled omits ifc label from result meta", func(t *testing.T) { deps := BaseDeps{ Client: mustNewGHClient(t, makeMockClient(false)), - Flags: FeatureFlags{InsidersMode: false}, } handler := serverTool.Handler(deps) @@ -535,8 +534,8 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode enabled on public repo emits public untrusted label", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(false)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(false)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -560,8 +559,8 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode enabled on private repo emits private trusted label", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(true)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(true)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -604,8 +603,8 @@ func Test_GetFileContents_IFC_InsidersMode(t *testing.T) { }, }) deps := BaseDeps{ - Client: mustNewGHClient(t, mockedClient), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, mockedClient), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) diff --git a/pkg/github/search.go b/pkg/github/search.go index 9d50a63103..9a8d182887 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -163,7 +163,7 @@ func SearchRepositories(t translations.TranslationHelperFunc) inventory.ServerTo } callResult := utils.NewToolResultText(string(r)) - if deps.GetFlags(ctx).InsidersMode { + if deps.IsFeatureEnabled(ctx, FeatureFlagIFCLabels) { attachSearchRepositoriesIFCLabel(result.Repositories, callResult) } return callResult, nil, nil diff --git a/pkg/github/search_test.go b/pkg/github/search_test.go index f1acec3e28..fa48bf19a1 100644 --- a/pkg/github/search_test.go +++ b/pkg/github/search_test.go @@ -207,7 +207,6 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode disabled omits ifc label", func(t *testing.T) { deps := BaseDeps{ Client: mustNewGHClient(t, makeMockClient([]repoFixture{{owner: "octocat", name: "public-repo"}})), - Flags: FeatureFlags{InsidersMode: false}, } handler := serverTool.Handler(deps) @@ -224,7 +223,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { {owner: "octocat", name: "public-a"}, {owner: "octocat", name: "public-b"}, })), - Flags: FeatureFlags{InsidersMode: true}, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -245,7 +244,7 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { {owner: "octocat", name: "private-repo", isPrivate: true}, {owner: "octocat", name: "public-repo"}, })), - Flags: FeatureFlags{InsidersMode: true}, + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) @@ -262,8 +261,8 @@ func Test_SearchRepositories_IFC_InsidersMode(t *testing.T) { t.Run("insiders mode empty results emits public untrusted", func(t *testing.T) { deps := BaseDeps{ - Client: mustNewGHClient(t, makeMockClient(nil)), - Flags: FeatureFlags{InsidersMode: true}, + Client: mustNewGHClient(t, makeMockClient(nil)), + featureChecker: featureCheckerFor(FeatureFlagIFCLabels), } handler := serverTool.Handler(deps) From f59fce9f8e4edfee2c55a8726bde738630269e1f Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 15:51:06 +0200 Subject: [PATCH 3/8] Simplify feature-flag handling: collapse CSV dual-variant + skip filtering when no checker (#2516) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: generic toolset+name sort, clarify feature flag intent Address review feedback on #2450: - Collapse the three near-identical sort helpers in pkg/inventory/filters.go into a generic sortByToolsetThenName so adding new inventory item types doesn't require copying the comparator. - Expand the doc comments on the three *WithoutFeatureFiltering helpers to spell out why they exist: HTTP mode builds a static (process-wide) inventory as an upper bound, but per-request feature flags from headers (X-MCP-Features, X-MCP-Insiders) are evaluated later, so feature-flagged variants must be preserved here. - Strengthen the doc comment on ResolveFeatureFlags to make the contract explicit: user-supplied flags are validated against AllowedFeatureFlags, but insiders expansion deliberately is not — InsidersFeatureFlags may include server-controlled flags that are not user-toggleable. CORS comments are intentionally left for the PR author. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(feature-flags): clarify allowed and insiders sets are independent Also add tests covering: - a user-toggleable flag (FeatureFlagIssuesGranular) that insiders does not turn on automatically - insiders mode not turning on user-only allowed flags Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(inventory): collapse three *WithoutFeatureFiltering helpers into StaticUpperBound The three parallel methods (AvailableToolsWithoutFeatureFiltering, AvailableResourceTemplatesWithoutFeatureFiltering, AvailablePromptsWithoutFeatureFiltering) were always called as a triple in exactly two places: HTTP buildStaticInventory and its test mirror. They exist because the dual-variant pattern (sibling tools with mirrored FeatureFlagEnable / FeatureFlagDisable on the same name, e.g. CSV output) makes feature filtering at static-build time impossible — both variants must be kept and resolved per-request. Replace the three with one method, Inventory.StaticUpperBound(ctx), that returns (tools, resources, prompts) and carries the rationale in its doc comment. Reduces API surface, eliminates the triplication, and makes the single "skip feature filtering" concept obvious to readers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: simplify feature-flag handling Two related simplifications, both about treating insiders as a meta flag that expands once at startup and then stops mattering: - Collapse CSV's dual-variant pattern into a single tool whose handler performs a runtime feature-flag check via deps.IsFeatureEnabled. CSV is a pure response-format toggle, not a schema change, so it does not need the dual-name pattern that genuine schema variants (granular issues/PRs) still use. - When no feature checker is installed, skip feature-flag filtering and return the full upper bound. The static HTTP inventory now uses plain AvailableTools/Resources/Prompts; the per-request inventory always installs a checker, so MCP registration (which serves a tool name once) always sees a deduplicated set. The bespoke StaticUpperBound helper and the isToolEnabledWithFeatureFlags split go away. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci(mcp-diff): add insiders + per-feature configs The mcp-diff matrix now includes: - --insiders (and --insiders --read-only) - one config per github.AllowedFeatureFlags entry, generated by script/print-mcp-diff-configs so new user-controllable flags get diffed automatically without editing the workflow Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(insiders): explain feature-flag resolution for contributors Adds a 'How feature flags are resolved' section covering: - Insiders is a meta flag, like 'all'/'default' for toolsets - User input -> allowlist filter -> insiders expansion -> server-side fallback (remote only) - AllowedFeatureFlags vs InsidersFeatureFlags are independent - How to add a new feature flag, including the TestGitHubPackageDoesNotReadInsidersMode guard Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(inventory): make feature-flag gating a regular ToolFilter Move tool feature-flag evaluation out of isToolEnabled and into a ToolFilter installed at the head of the pipeline by Build() when WithFeatureChecker received a non-nil checker. The 'no checker = no filtering' contract is now expressed structurally (the filter isn't installed) instead of by a runtime nil check inside the helper. Resources and prompts have no filter pipeline, so they call the now-pure featureFlagAllowed helper behind an explicit r.featureChecker != nil guard at the iteration site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf(inventory): cache extracted toolset IDs in sort comparator Avoid evaluating the extractor closures up to three times per comparison. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/mcp-diff.yml | 35 +++--- docs/insiders-features.md | 32 +++++ pkg/github/csv_output.go | 29 ++--- pkg/github/csv_output_test.go | 128 ++++++++++--------- pkg/github/feature_flags.go | 14 ++- pkg/github/feature_flags_test.go | 12 ++ pkg/github/tools.go | 2 +- pkg/http/handler.go | 20 +-- pkg/http/handler_test.go | 9 +- pkg/inventory/builder.go | 28 ++++- pkg/inventory/filters.go | 169 ++++++++++++-------------- pkg/inventory/registry_test.go | 51 ++++---- script/print-mcp-diff-configs/main.go | 61 ++++++++++ 13 files changed, 357 insertions(+), 233 deletions(-) create mode 100644 script/print-mcp-diff-configs/main.go diff --git a/.github/workflows/mcp-diff.yml b/.github/workflows/mcp-diff.yml index bb6341c096..b9c7199e56 100644 --- a/.github/workflows/mcp-diff.yml +++ b/.github/workflows/mcp-diff.yml @@ -19,32 +19,35 @@ jobs: with: fetch-depth: 0 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Build UI uses: ./.github/actions/build-ui + - name: Generate diff configurations + id: configs + # The generator imports pkg/github so any new entry in + # AllowedFeatureFlags is automatically diffed without touching this + # workflow. See script/print-mcp-diff-configs/main.go. + run: | + { + echo 'configurations<> "$GITHUB_OUTPUT" + - name: Run MCP Server Diff uses: SamMorrowDrums/mcp-server-diff@v2.3.5 with: - setup_go: "true" + setup_go: "false" install_command: go mod download start_command: go run ./cmd/github-mcp-server stdio env_vars: | GITHUB_PERSONAL_ACCESS_TOKEN=test-token - configurations: | - [ - {"name": "default", "args": ""}, - {"name": "read-only", "args": "--read-only"}, - {"name": "toolsets-repos", "args": "--toolsets=repos"}, - {"name": "toolsets-issues", "args": "--toolsets=issues"}, - {"name": "toolsets-context", "args": "--toolsets=context"}, - {"name": "toolsets-pull_requests", "args": "--toolsets=pull_requests"}, - {"name": "toolsets-repos,issues", "args": "--toolsets=repos,issues"}, - {"name": "toolsets-issues,context", "args": "--toolsets=issues,context"}, - {"name": "toolsets-all", "args": "--toolsets=all"}, - {"name": "tools-get_me", "args": "--tools=get_me"}, - {"name": "tools-get_me,list_issues", "args": "--tools=get_me,list_issues"}, - {"name": "toolsets-repos+read-only", "args": "--toolsets=repos --read-only"} - ] + configurations: ${{ steps.configs.outputs.configurations }} - name: Add interpretation note if: always() diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 59147c4f58..90afe7219e 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -67,3 +67,35 @@ github-mcp-server stdio --features csv_output ``` Because this changes list tool response shape, clients that require JSON list responses should avoid enabling this feature. + +--- + +## How feature flags are resolved + +> [!NOTE] +> This section is for contributors. End users only need the table at the top of this page. + +Insiders is a **meta feature flag** — the same shape as `default` or `all` for toolsets. It expands once at startup into a curated set of individual feature flags, and from that point on every code path keys off concrete flags, never `InsidersMode` directly. New experimental work should always get its own flag and then be added to the insiders expansion list, never folded into `insiders` as a catch-all. + +### Resolution order + +1. **User input.** Users may opt into specific features: + - Local server: `--features=,` CLI flag (or `GITHUB_FEATURES` env var). + - Self-hosted HTTP server: `X-MCP-Features: ,` request header. +2. **Allowlist filter.** User-supplied flags are filtered against [`AllowedFeatureFlags`](../pkg/github/feature_flags.go). Anything not on the allowlist is silently dropped — flags missing from the allowlist can only be turned on by remote-server feature management, not by end users. +3. **Insiders expansion.** If insiders mode is on (`--insiders`, `/insiders` route, or `X-MCP-Insiders: true`), every flag in [`InsidersFeatureFlags`](../pkg/github/feature_flags.go) is unioned in. The insiders expansion is **not** re-validated against the allowlist — insiders is a server-controlled switch that can reach internal-only flags. +4. **Server-side fallback (remote server only).** Any flag not yet decided falls back to the remote server's feature manager, which can roll a feature out independently of user input or insiders membership. + +`AllowedFeatureFlags` and `InsidersFeatureFlags` are deliberately independent sets: + +- A flag in **`AllowedFeatureFlags` only** is a regular opt-in: users can turn it on, but insiders does not auto-enable it. Granular issues/PRs flags work this way. +- A flag in **`InsidersFeatureFlags` only** is reachable through insiders (and remote-server rollouts), but cannot be enabled by user input. Internal-only experiments work this way. +- A flag in **both** is opt-in for end users *and* automatically on under insiders. + +### Adding a new feature flag + +1. Add a constant in `pkg/github/feature_flags.go`. +2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via `--features` / `X-MCP-Features`. +3. Add it to `InsidersFeatureFlags` if insiders mode should turn it on automatically. +4. Gate the behavior on the concrete flag (`deps.IsFeatureEnabled(ctx, FeatureFlagX)`), never on `cfg.InsidersMode`. There is a `TestGitHubPackageDoesNotReadInsidersMode` guard test that fails if `pkg/github` reads `InsidersMode` directly. +5. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`. diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index 5c06a1c8c7..cb70e32d77 100644 --- a/pkg/github/csv_output.go +++ b/pkg/github/csv_output.go @@ -42,24 +42,18 @@ type csvOutputDocument struct { rows []map[string]string } -func withCSVOutputVariants(tools []inventory.ServerTool) []inventory.ServerTool { - result := make([]inventory.ServerTool, 0, len(tools)) - for _, tool := range tools { - if !isCSVOutputTool(tool) { - result = append(result, tool) +// withCSVOutput wraps the handler of every default-toolset list_* tool so that, +// at request time, it checks the csv_output feature flag and converts the JSON +// text response to CSV when enabled. The tool's schema, name, and scope are +// unchanged — only the response payload format differs. +func withCSVOutput(tools []inventory.ServerTool) []inventory.ServerTool { + for i := range tools { + if !isCSVOutputTool(tools[i]) { continue } - - jsonOnly := tool - jsonOnly.FeatureFlagDisable = FeatureFlagCSVOutput - result = append(result, jsonOnly) - - csvCapable := tool - csvCapable.FeatureFlagEnable = FeatureFlagCSVOutput - csvCapable.HandlerFunc = wrapHandlerWithCSVOutput(tool.HandlerFunc) - result = append(result, csvCapable) + tools[i].HandlerFunc = wrapHandlerWithCSVOutput(tools[i].HandlerFunc) } - return result + return tools } func isCSVOutputTool(tool inventory.ServerTool) bool { @@ -75,12 +69,15 @@ func isCSVOutputTool(tool inventory.ServerTool) bool { func wrapHandlerWithCSVOutput(next inventory.HandlerFunc) inventory.HandlerFunc { return func(deps any) mcp.ToolHandler { handler := next(deps) + csvDeps, _ := deps.(ToolDependencies) return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { result, err := handler(ctx, req) if err != nil || result == nil || result.IsError { return result, err } - + if csvDeps == nil || !csvDeps.IsFeatureEnabled(ctx, FeatureFlagCSVOutput) { + return result, nil + } return convertJSONTextResultToCSV(result), nil } } diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go index 80ff8a7892..d0bef38938 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -14,55 +14,55 @@ import ( "github.com/stretchr/testify/require" ) -func TestCSVOutputVariantsAreFeatureGated(t *testing.T) { +func TestCSVOutputAppliedToDefaultListTools(t *testing.T) { listTool := testCSVOutputTool("list_things", `[{"number":1}]`) getTool := testCSVOutputTool("get_thing", `{"number":1}`) - tools := withCSVOutputVariants([]inventory.ServerTool{listTool, getTool}) - require.Len(t, tools, 3) + tools := withCSVOutput([]inventory.ServerTool{listTool, getTool}) + require.Len(t, tools, 2) - inv := buildCSVOutputInventory(t, tools, false) - available := inv.AvailableTools(context.Background()) - require.Len(t, available, 2) + // CSV mode does not introduce variants or change tool gating; both tools + // remain visible regardless of feature flag state. + for _, csvOutputEnabled := range []bool{false, true} { + inv := buildCSVOutputInventory(t, tools, csvOutputEnabled) + available := inv.AvailableTools(context.Background()) + require.Len(t, available, 2) - jsonOnly := requireToolByName(t, available, "list_things") - assert.Empty(t, jsonOnly.FeatureFlagEnable) - assert.Equal(t, FeatureFlagCSVOutput, jsonOnly.FeatureFlagDisable) + listing := requireToolByName(t, available, "list_things") + assert.Empty(t, listing.FeatureFlagEnable) + assert.Empty(t, listing.FeatureFlagDisable) - getThing := requireToolByName(t, available, "get_thing") - assert.Empty(t, getThing.FeatureFlagEnable) - assert.Empty(t, getThing.FeatureFlagDisable) - - inv = buildCSVOutputInventory(t, tools, true) - available = inv.AvailableTools(context.Background()) - require.Len(t, available, 2) - - csvCapable := requireToolByName(t, available, "list_things") - assert.Equal(t, FeatureFlagCSVOutput, csvCapable.FeatureFlagEnable) - assert.Empty(t, csvCapable.FeatureFlagDisable) + getting := requireToolByName(t, available, "get_thing") + assert.Empty(t, getting.FeatureFlagEnable) + assert.Empty(t, getting.FeatureFlagDisable) + } } -func TestCSVOutputVariantsOnlyApplyToDefaultToolsets(t *testing.T) { +func TestCSVOutputOnlyAppliesToDefaultToolsets(t *testing.T) { nonDefaultListTool := testCSVOutputToolWithToolset("list_discussions", `[{"number":1}]`, ToolsetMetadataDiscussions) - tools := withCSVOutputVariants([]inventory.ServerTool{nonDefaultListTool}) + tools := withCSVOutput([]inventory.ServerTool{nonDefaultListTool}) require.Len(t, tools, 1) - assert.Empty(t, tools[0].FeatureFlagEnable) - assert.Empty(t, tools[0].FeatureFlagDisable) + // Non-default toolset list tools are not wrapped: even with the flag on, + // the response stays in JSON form. + deps := newCSVOutputTestDeps(true) + result, err := tools[0].Handler(deps)(ContextWithDeps(context.Background(), deps), testCSVOutputRequest()) + require.NoError(t, err) + assert.JSONEq(t, `[{"number":1}]`, textResult(t, result)) } -func TestCSVOutputVariantDoesNotExposeFormatParameter(t *testing.T) { - tools := withCSVOutputVariants([]inventory.ServerTool{testCSVOutputTool("list_things", `[{"number":1}]`)}) - csvCapable := requireCSVOutputVariant(t, tools) +func TestCSVOutputDoesNotExposeFormatParameter(t *testing.T) { + tools := withCSVOutput([]inventory.ServerTool{testCSVOutputTool("list_things", `[{"number":1}]`)}) + require.Len(t, tools, 1) - schema, ok := csvCapable.Tool.InputSchema.(*jsonschema.Schema) + schema, ok := tools[0].Tool.InputSchema.(*jsonschema.Schema) require.True(t, ok) assert.NotContains(t, schema.Properties, "output_format") } -func TestCSVOutputVariantConvertsJSONTextToCSV(t *testing.T) { - tools := withCSVOutputVariants([]inventory.ServerTool{ +func TestCSVOutputConvertsJSONTextToCSVWhenFlagOn(t *testing.T) { + tools := withCSVOutput([]inventory.ServerTool{ testCSVOutputTool("list_things", `[ { "number": 1, @@ -72,10 +72,10 @@ func TestCSVOutputVariantConvertsJSONTextToCSV(t *testing.T) { } ]`), }) - inv := buildCSVOutputInventory(t, tools, true) - csvCapable := requireToolByName(t, inv.AvailableTools(context.Background()), "list_things") + require.Len(t, tools, 1) - result, err := csvCapable.Handler(nil)(context.Background(), testCSVOutputRequest()) + deps := newCSVOutputTestDeps(true) + result, err := tools[0].Handler(deps)(ContextWithDeps(context.Background(), deps), testCSVOutputRequest()) require.NoError(t, err) require.NotNil(t, result) require.False(t, result.IsError) @@ -92,6 +92,22 @@ func TestCSVOutputVariantConvertsJSONTextToCSV(t *testing.T) { assert.Equal(t, "octocat", row["user.login"]) } +func TestCSVOutputPreservesOriginalJSONWhenFlagOff(t *testing.T) { + const jsonResponse = `[{"number":1,"user":{"login":"octocat"}}]` + tools := withCSVOutput([]inventory.ServerTool{testCSVOutputTool("list_things", jsonResponse)}) + require.Len(t, tools, 1) + + deps := newCSVOutputTestDeps(false) + result, err := tools[0].Handler(deps)(ContextWithDeps(context.Background(), deps), testCSVOutputRequest()) + require.NoError(t, err) + require.NotNil(t, result) + + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.JSONEq(t, jsonResponse, text.Text) +} + func TestCSVOutputVariantMovesMetadataToPreamble(t *testing.T) { csvText, err := jsonTextToCSV(`{ "issues": [ @@ -118,22 +134,6 @@ func TestCSVOutputVariantMovesMetadataToPreamble(t *testing.T) { assert.NotContains(t, row, "totalCount") } -func TestJSONOnlyVariantPreservesOriginalJSONText(t *testing.T) { - const jsonResponse = `[{"number":1,"user":{"login":"octocat"}}]` - tools := withCSVOutputVariants([]inventory.ServerTool{testCSVOutputTool("list_things", jsonResponse)}) - inv := buildCSVOutputInventory(t, tools, false) - jsonOnly := requireToolByName(t, inv.AvailableTools(context.Background()), "list_things") - - result, err := jsonOnly.Handler(nil)(context.Background(), testCSVOutputRequest()) - require.NoError(t, err) - require.NotNil(t, result) - - require.Len(t, result.Content, 1) - text, ok := result.Content[0].(*mcp.TextContent) - require.True(t, ok) - assert.JSONEq(t, jsonResponse, text.Text) -} - func TestJSONTextToCSVFlattensPrimaryRows(t *testing.T) { csvText, err := jsonTextToCSV(`{ "discussions": [ @@ -329,40 +329,38 @@ func testCSVOutputToolWithToolset(name string, response string, toolset inventor } } -func buildCSVOutputInventory(t *testing.T, tools []inventory.ServerTool, csvOutputEnabled bool) *inventory.Inventory { +func buildCSVOutputInventory(t *testing.T, tools []inventory.ServerTool, _ bool) *inventory.Inventory { t.Helper() inv, err := inventory.NewBuilder(). SetTools(tools). - WithFeatureChecker(func(_ context.Context, flagName string) (bool, error) { - return flagName == FeatureFlagCSVOutput && csvOutputEnabled, nil - }). Build() require.NoError(t, err) return inv } -func requireToolByName(t *testing.T, tools []inventory.ServerTool, name string) inventory.ServerTool { - t.Helper() +func newCSVOutputTestDeps(csvOutputEnabled bool) ToolDependencies { + return csvOutputTestDeps{stubDeps: stubDeps{obsv: stubExporters()}, csvOn: csvOutputEnabled} +} - for _, tool := range tools { - if tool.Tool.Name == name { - return tool - } - } - require.Failf(t, "tool not found", "tool %q not found", name) - return inventory.ServerTool{} +type csvOutputTestDeps struct { + stubDeps + csvOn bool } -func requireCSVOutputVariant(t *testing.T, tools []inventory.ServerTool) inventory.ServerTool { +func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag string) bool { + return flag == FeatureFlagCSVOutput && d.csvOn +} + +func requireToolByName(t *testing.T, tools []inventory.ServerTool, name string) inventory.ServerTool { t.Helper() for _, tool := range tools { - if tool.FeatureFlagEnable == FeatureFlagCSVOutput { + if tool.Tool.Name == name { return tool } } - require.Fail(t, "CSV output variant not found") + require.Failf(t, "tool not found", "tool %q not found", name) return inventory.ServerTool{} } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 9adfa38d2a..19399e7acf 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -38,8 +38,18 @@ type FeatureFlags struct { } // ResolveFeatureFlags computes the effective set of enabled feature flags by: -// 1. Taking explicitly enabled features validated against AllowedFeatureFlags -// 2. Adding features enabled by insiders mode from InsidersFeatureFlags +// 1. Taking the user-supplied flags (from --features or X-MCP-Features) and +// keeping only those present in AllowedFeatureFlags. Unknown or unsafe +// flags from request input are silently dropped here. +// 2. If insiders mode is on, unioning in every flag from InsidersFeatureFlags. +// Insiders is a server-controlled meta switch, so its expansion is NOT +// re-validated against AllowedFeatureFlags. +// +// AllowedFeatureFlags and InsidersFeatureFlags are independent sets: +// - A flag in AllowedFeatureFlags but not InsidersFeatureFlags is a regular +// opt-in flag that insiders mode does not turn on automatically. +// - A flag in InsidersFeatureFlags but not AllowedFeatureFlags is reachable +// only through insiders mode and cannot be enabled by user input. // // Returns a set (map) for O(1) lookup by the feature checker. func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool { diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 9bc1be473f..9f31ada382 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -184,6 +184,18 @@ func TestResolveFeatureFlags(t *testing.T) { expectedFlags: []string{MCPAppsFeatureFlag}, unexpectedFlags: []string{"unknown_flag"}, }, + { + name: "user-only flags can be enabled but are not turned on by insiders", + enabledFeatures: []string{FeatureFlagIssuesGranular}, + insidersMode: false, + expectedFlags: []string{FeatureFlagIssuesGranular}, + }, + { + name: "insiders does not enable user-only allowed flags", + enabledFeatures: nil, + insidersMode: true, + unexpectedFlags: []string{FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular}, + }, { name: "explicit plus insiders deduplicates", enabledFeatures: []string{MCPAppsFeatureFlag}, diff --git a/pkg/github/tools.go b/pkg/github/tools.go index e7530b672d..6496c85f81 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -167,7 +167,7 @@ var ( // AllTools returns all tools with their embedded toolset metadata. // Tool functions return ServerTool directly with toolset info. func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { - return withCSVOutputVariants([]inventory.ServerTool{ + return withCSVOutput([]inventory.ServerTool{ // Context tools GetMe(t), GetTeams(t), diff --git a/pkg/http/handler.go b/pkg/http/handler.go index dd6161d52f..e585a86569 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -249,7 +249,7 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc { // Build the static tool/resource/prompt universe from CLI flags. // This is done once at startup and captured in the closure. - staticTools, staticResources, staticPrompts := buildStaticInventory(cfg, t, featureChecker) + staticTools, staticResources, staticPrompts := buildStaticInventory(cfg, t) hasStaticFilters := hasStaticConfig(cfg) // Pre-compute valid tool names for filtering per-request tool headers. @@ -325,15 +325,19 @@ func hasStaticConfig(cfg *ServerConfig) bool { } // buildStaticInventory pre-filters the full tool/resource/prompt universe using -// the static CLI flags (--toolsets, --read-only, --exclude-tools, etc.). -// The returned slices serve as the upper bound for per-request inventory builders. -func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { +// the static config (toolsets, read-only, --tools, --exclude-tools). It does +// NOT install a feature checker: HTTP feature flags can come from per-request +// context (/insiders, X-MCP-Features), so dual-name feature variants — for +// example the granular issues/PRs tools that share a name with their +// non-granular siblings — must be carried through to the per-request +// inventory, which then installs a checker and resolves the flag before +// registering tools with the MCP server. +func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { if !hasStaticConfig(cfg) { return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) } b := github.NewInventory(t). - WithFeatureChecker(featureChecker). WithReadOnly(cfg.ReadOnly). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) @@ -352,12 +356,8 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun return github.AllTools(t), github.AllResources(t), github.AllPrompts(t) } - // Static filtering defines an upper bound for all requests. Do not apply - // feature flags here: HTTP feature flags can come from request context - // (/insiders, X-MCP-Features), so variants must be preserved until the - // per-request inventory evaluates them. ctx := context.Background() - return inv.AvailableToolsWithoutFeatureFiltering(ctx), inv.AvailableResourceTemplatesWithoutFeatureFiltering(ctx), inv.AvailablePromptsWithoutFeatureFiltering(ctx) + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) } // InventoryFiltersForRequest applies filters to the inventory builder diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index dfb402093b..74e28a6e44 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -554,7 +554,7 @@ func TestStaticConfigEnforcement(t *testing.T) { require.NoError(t, err) // Build static tools the same way the production code does - staticTools, staticResources, staticPrompts := buildStaticInventoryFromTools(tt.config, tools, featureChecker) + staticTools, staticResources, staticPrompts := buildStaticInventoryFromTools(tt.config, tools) hasStatic := hasStaticConfig(tt.config) validToolNames := make(map[string]bool, len(staticTools)) @@ -640,7 +640,7 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { cfg := &ServerConfig{Version: "test", EnabledToolsets: []string{"issues"}} featureChecker := createHTTPFeatureChecker(nil, false) - staticTools, _, _ := buildStaticInventoryFromTools(cfg, tools, featureChecker) + staticTools, _, _ := buildStaticInventoryFromTools(cfg, tools) require.Len(t, staticTools, 2, "static upper bounds should preserve both feature variants") inv, err := inventory.NewBuilder(). @@ -754,14 +754,13 @@ func TestContentTypeHandling(t *testing.T) { // buildStaticInventoryFromTools is a test helper that mirrors buildStaticInventory // but uses the provided mock tools instead of calling github.AllTools. -func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTool, featureChecker inventory.FeatureFlagChecker) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { +func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTool) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) { if !hasStaticConfig(cfg) { return tools, nil, nil } b := inventory.NewBuilder(). SetTools(tools). - WithFeatureChecker(featureChecker). WithReadOnly(cfg.ReadOnly). WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) @@ -779,7 +778,7 @@ func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTo } ctx := context.Background() - return inv.AvailableToolsWithoutFeatureFiltering(ctx), inv.AvailableResourceTemplatesWithoutFeatureFiltering(ctx), inv.AvailablePromptsWithoutFeatureFiltering(ctx) + return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx) } func TestCrossOriginProtection(t *testing.T) { diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 2642c6127a..9ecaca1f57 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -127,8 +127,20 @@ func (b *Builder) WithTools(toolNames []string) *Builder { // WithFeatureChecker sets the feature flag checker function. // The checker receives a context (for actor extraction) and feature flag name, -// returns (enabled, error). If error occurs, it will be logged and treated as false. -// If checker is nil, all feature flag checks return false. +// and returns (enabled, error). Errors are logged and treated as "not enabled". +// +// When the checker is non-nil, Build() installs a feature-flag ToolFilter +// at the head of the filter pipeline so that tools annotated with +// FeatureFlagEnable / FeatureFlagDisable are gated accordingly. Resources +// and prompts use the same checker via an explicit guard at their iteration +// site. +// +// When the checker is nil, no feature-flag filter is installed; tools, +// resources, and prompts pass through feature-flag gating unchanged. The +// per-request inventory in HTTP mode must always install a checker so that +// MCP registration (which can only serve a given tool name once) sees a +// deduplicated set of dual-name variants. +// // Returns self for chaining. func (b *Builder) WithFeatureChecker(checker FeatureFlagChecker) *Builder { b.featureChecker = checker @@ -200,6 +212,16 @@ func cleanTools(tools []string) []string { func (b *Builder) Build() (*Inventory, error) { tools := b.tools + // Install the feature-flag filter at the head of the pipeline so that + // flag-gated tools are excluded before any user-supplied WithFilter sees + // them. Doing this in Build() (rather than inside WithFeatureChecker) + // keeps the install idempotent — repeated WithFeatureChecker calls + // replace the checker without stacking duplicate filters. + filters := b.filters + if b.featureChecker != nil { + filters = append([]ToolFilter{createFeatureFlagFilter(b.featureChecker)}, filters...) + } + r := &Inventory{ tools: tools, resourceTemplates: b.resourceTemplates, @@ -207,7 +229,7 @@ func (b *Builder) Build() (*Inventory, error) { deprecatedAliases: b.deprecatedAliases, readOnly: b.readOnly, featureChecker: b.featureChecker, - filters: b.filters, + filters: filters, } // Process toolsets and pre-compute metadata in a single pass diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 20a157de63..e2effd8ca7 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -35,33 +35,56 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool return enabled } -// isFeatureFlagAllowed checks if an item passes feature flag filtering. -// - If FeatureFlagEnable is set, the item is only allowed if the flag is enabled -// - If FeatureFlagDisable is set, the item is excluded if the flag is enabled -func (r *Inventory) isFeatureFlagAllowed(ctx context.Context, enableFlag, disableFlag string) bool { - // Check enable flag - item requires this flag to be on - if enableFlag != "" && !r.checkFeatureFlag(ctx, enableFlag) { +// featureFlagAllowed reports whether an item with the given enable/disable +// flag pair is permitted under the supplied checker. The checker must be +// non-nil — callers that don't want feature filtering should not call this at +// all (this is also the contract for createFeatureFlagFilter, which is only +// installed when WithFeatureChecker received a non-nil checker). +// +// - If FeatureFlagEnable is set, the item is only allowed if the flag is enabled. +// - If FeatureFlagDisable is set, the item is excluded if the flag is enabled. +func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag, disableFlag string) bool { + // Error semantics match the previous checkFeatureFlag helper: a checker + // error is logged and treated as "flag not enabled". So an enable-flag + // check on error excludes the tool, but a disable-flag check on error + // keeps it (the disable condition wasn't met). + check := func(flag string) bool { + enabled, err := checker(ctx, flag) + if err != nil { + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flag, err) + return false + } + return enabled + } + if enableFlag != "" && !check(enableFlag) { return false } - // Check disable flag - item is excluded if this flag is on - if disableFlag != "" && r.checkFeatureFlag(ctx, disableFlag) { + if disableFlag != "" && check(disableFlag) { return false } return true } +// createFeatureFlagFilter returns a ToolFilter that gates tools on their +// FeatureFlagEnable / FeatureFlagDisable annotations using the given checker. +// Builder.Build() installs this filter exactly once when WithFeatureChecker +// has been called with a non-nil checker, so "no feature filtering" is +// expressed structurally — by the absence of the filter — rather than by a +// runtime nil check inside the filter itself. +func createFeatureFlagFilter(checker FeatureFlagChecker) ToolFilter { + return func(ctx context.Context, tool *ServerTool) (bool, error) { + return featureFlagAllowed(ctx, checker, tool.FeatureFlagEnable, tool.FeatureFlagDisable), nil + } +} + // isToolEnabled checks if a specific tool is enabled based on current filters. // Filter evaluation order: // 1. Tool.Enabled (tool self-filtering) -// 2. FeatureFlagEnable/FeatureFlagDisable -// 3. Read-only filter -// 4. Builder filters (via WithFilter) -// 5. Toolset/additional tools +// 2. Read-only filter +// 3. Builder filters (via WithFilter; the feature-flag filter, when +// installed via WithFeatureChecker, runs as part of this step) +// 4. Toolset/additional tools func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { - return r.isToolEnabledWithFeatureFlags(ctx, tool, true) -} - -func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *ServerTool, checkFeatureFlags bool) bool { // 1. Check tool's own Enabled function first if tool.Enabled != nil { enabled, err := tool.Enabled(ctx) @@ -73,15 +96,11 @@ func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *Ser return false } } - // 2. Check feature flags - if checkFeatureFlags && !r.isFeatureFlagAllowed(ctx, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { - return false - } - // 3. Check read-only filter (applies to all tools) + // 2. Check read-only filter (applies to all tools) if r.readOnly && !tool.IsReadOnly() { return false } - // 4. Apply builder filters + // 3. Apply builder filters (includes the feature-flag filter when set) for _, filter := range r.filters { allowed, err := filter(ctx, tool) if err != nil { @@ -92,26 +111,38 @@ func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *Ser return false } } - // 5. Check if tool is in additionalTools (bypasses toolset filter) + // 4. Check if tool is in additionalTools (bypasses toolset filter) if r.additionalTools != nil && r.additionalTools[tool.Tool.Name] { return true } - // 5. Check toolset filter + // 4. Check toolset filter if !r.isToolsetEnabled(tool.Toolset.ID) { return false } return true } -func sortTools(tools []ServerTool) { - sort.Slice(tools, func(i, j int) bool { - if tools[i].Toolset.ID != tools[j].Toolset.ID { - return tools[i].Toolset.ID < tools[j].Toolset.ID +// sortByToolsetThenName sorts items deterministically by their toolset ID, +// breaking ties by name. The two extractor closures keep this generic helper +// independent of the concrete inventory item shape (tools, resource templates, +// prompts). +func sortByToolsetThenName[T any](items []T, toolsetID func(T) ToolsetID, name func(T) string) { + sort.Slice(items, func(i, j int) bool { + idI, idJ := toolsetID(items[i]), toolsetID(items[j]) + if idI != idJ { + return idI < idJ } - return tools[i].Tool.Name < tools[j].Tool.Name + return name(items[i]) < name(items[j]) }) } +func sortTools(tools []ServerTool) { + sortByToolsetThenName(tools, + func(t ServerTool) ToolsetID { return t.Toolset.ID }, + func(t ServerTool) string { return t.Tool.Name }, + ) +} + // AvailableTools returns the tools that pass all current filters, // sorted deterministically by toolset ID, then tool name. // The context is used for feature flag evaluation. @@ -130,29 +161,11 @@ func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool { return result } -// AvailableToolsWithoutFeatureFiltering returns tools that pass every filter -// except FeatureFlagEnable/FeatureFlagDisable. -func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) []ServerTool { - var result []ServerTool - for i := range r.tools { - tool := &r.tools[i] - if r.isToolEnabledWithFeatureFlags(ctx, tool, false) { - result = append(result, *tool) - } - } - - sortTools(result) - - return result -} - func sortResourceTemplates(resourceTemplates []ServerResourceTemplate) { - sort.Slice(resourceTemplates, func(i, j int) bool { - if resourceTemplates[i].Toolset.ID != resourceTemplates[j].Toolset.ID { - return resourceTemplates[i].Toolset.ID < resourceTemplates[j].Toolset.ID - } - return resourceTemplates[i].Template.Name < resourceTemplates[j].Template.Name - }) + sortByToolsetThenName(resourceTemplates, + func(r ServerResourceTemplate) ToolsetID { return r.Toolset.ID }, + func(r ServerResourceTemplate) string { return r.Template.Name }, + ) } // AvailableResourceTemplates returns resource templates that pass all current filters, @@ -162,8 +175,11 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso var result []ServerResourceTemplate for i := range r.resourceTemplates { res := &r.resourceTemplates[i] - // Check feature flags - if !r.isFeatureFlagAllowed(ctx, res.FeatureFlagEnable, res.FeatureFlagDisable) { + // Resources have no filter pipeline, so feature gating runs inline. + // The featureChecker != nil guard mirrors the structural "no checker + // = no filtering" contract used for tools (where the absence of a + // pipeline step expresses the same thing). + if r.featureChecker != nil && !featureFlagAllowed(ctx, r.featureChecker, res.FeatureFlagEnable, res.FeatureFlagDisable) { continue } if r.isToolsetEnabled(res.Toolset.ID) { @@ -177,29 +193,11 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso return result } -// AvailableResourceTemplatesWithoutFeatureFiltering returns resource templates -// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. -func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.Context) []ServerResourceTemplate { - var result []ServerResourceTemplate - for i := range r.resourceTemplates { - res := &r.resourceTemplates[i] - if r.isToolsetEnabled(res.Toolset.ID) { - result = append(result, *res) - } - } - - sortResourceTemplates(result) - - return result -} - func sortPrompts(prompts []ServerPrompt) { - sort.Slice(prompts, func(i, j int) bool { - if prompts[i].Toolset.ID != prompts[j].Toolset.ID { - return prompts[i].Toolset.ID < prompts[j].Toolset.ID - } - return prompts[i].Prompt.Name < prompts[j].Prompt.Name - }) + sortByToolsetThenName(prompts, + func(p ServerPrompt) ToolsetID { return p.Toolset.ID }, + func(p ServerPrompt) string { return p.Prompt.Name }, + ) } // AvailablePrompts returns prompts that pass all current filters, @@ -209,8 +207,9 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { var result []ServerPrompt for i := range r.prompts { prompt := &r.prompts[i] - // Check feature flags - if !r.isFeatureFlagAllowed(ctx, prompt.FeatureFlagEnable, prompt.FeatureFlagDisable) { + // Prompts have no filter pipeline; see AvailableResourceTemplates for + // the rationale behind the explicit nil guard. + if r.featureChecker != nil && !featureFlagAllowed(ctx, r.featureChecker, prompt.FeatureFlagEnable, prompt.FeatureFlagDisable) { continue } if r.isToolsetEnabled(prompt.Toolset.ID) { @@ -224,22 +223,6 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { return result } -// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every filter -// except FeatureFlagEnable/FeatureFlagDisable. -func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt { - var result []ServerPrompt - for i := range r.prompts { - prompt := &r.prompts[i] - if r.isToolsetEnabled(prompt.Toolset.ID) { - result = append(result, *prompt) - } - } - - sortPrompts(result) - - return result -} - // filterToolsByName returns tools matching the given name, checking deprecated aliases. // Uses linear scan - optimized for single-lookup per-request scenarios (ForMCPRequest). // Returns ALL tools matching the name to support feature-flagged tool variants diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 8e35861f15..75de9c574a 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1057,23 +1057,23 @@ func TestFeatureFlagEnable(t *testing.T) { mockToolWithFlags("needs_flag", "toolset1", true, "my_feature", ""), } - // Without feature checker, tool with FeatureFlagEnable should be excluded + // Without feature checker, feature-flag filtering is skipped: both tools pass reg := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"})) available := reg.AvailableTools(context.Background()) - if len(available) != 1 { - t.Fatalf("Expected 1 tool without feature checker, got %d", len(available)) - } - if available[0].Tool.Name != "always_available" { - t.Errorf("Expected always_available, got %s", available[0].Tool.Name) + if len(available) != 2 { + t.Fatalf("Expected 2 tools without feature checker (filtering skipped), got %d", len(available)) } - // With feature checker returning false, tool should still be excluded + // With feature checker returning false, FeatureFlagEnable tool is excluded checkerFalse := func(_ context.Context, _ string) (bool, error) { return false, nil } regFalse := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerFalse)) availableFalse := regFalse.AvailableTools(context.Background()) if len(availableFalse) != 1 { t.Fatalf("Expected 1 tool with false checker, got %d", len(availableFalse)) } + if availableFalse[0].Tool.Name != "always_available" { + t.Errorf("Expected always_available, got %s", availableFalse[0].Tool.Name) + } // With feature checker returning true for "my_feature", tool should be included checkerTrue := func(_ context.Context, flag string) (bool, error) { @@ -1167,11 +1167,11 @@ func TestFeatureFlagResources(t *testing.T) { }, } - // Without checker, resource with enable flag should be excluded + // Without checker, feature-flag filtering is skipped: both resources pass reg := mustBuild(t, NewBuilder().SetResources(resources).WithToolsets([]string{"all"})) available := reg.AvailableResourceTemplates(context.Background()) - if len(available) != 1 { - t.Fatalf("Expected 1 resource without checker, got %d", len(available)) + if len(available) != 2 { + t.Fatalf("Expected 2 resources without checker (filtering skipped), got %d", len(available)) } // With checker returning true, both should be included @@ -1192,11 +1192,11 @@ func TestFeatureFlagPrompts(t *testing.T) { }, } - // Without checker, prompt with enable flag should be excluded + // Without checker, feature-flag filtering is skipped: both prompts pass reg := mustBuild(t, NewBuilder().SetPrompts(prompts).WithToolsets([]string{"all"})) available := reg.AvailablePrompts(context.Background()) - if len(available) != 1 { - t.Fatalf("Expected 1 prompt without checker, got %d", len(available)) + if len(available) != 2 { + t.Fatalf("Expected 2 prompts without checker (filtering skipped), got %d", len(available)) } // With checker returning true, both should be included @@ -1482,9 +1482,11 @@ func TestEnabledAndFeatureFlagInteraction(t *testing.T) { } // Feature flag not enabled - tool should be excluded despite Enabled returning true + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } reg1 := mustBuild(t, NewBuilder(). SetTools([]ServerTool{tool}). - WithToolsets([]string{"all"})) + WithToolsets([]string{"all"}). + WithFeatureChecker(checkerOff)) available1 := reg1.AvailableTools(context.Background()) if len(available1) != 0 { t.Error("Tool should be excluded when feature flag is not enabled") @@ -1650,10 +1652,10 @@ func TestFilteredToolsMatchesAvailableTools(t *testing.T) { func TestFilteringOrder(t *testing.T) { // Test that filters are applied in the correct order: // 1. Tool.Enabled - // 2. Feature flags - // 3. Read-only - // 4. Builder filters - // 5. Toolset/additional tools + // 2. Read-only + // 3. Builder filters (feature-flag filter is at the head of this list + // when WithFeatureChecker is set) + // 4. Toolset/additional tools callOrder := []string{} @@ -1686,8 +1688,9 @@ func TestFilteringOrder(t *testing.T) { _ = reg.AvailableTools(context.Background()) - // Expected order: Enabled, FeatureFlag, ReadOnly (stops here because it's write tool) - expectedOrder := []string{"Enabled", "FeatureFlag"} + // Expected order: Enabled, then Read-only stops (write tool, read-only mode); + // neither the feature-flag filter nor the user filter is reached. + expectedOrder := []string{"Enabled"} if len(callOrder) != len(expectedOrder) { t.Errorf("Expected %d checks, got %d: %v", len(expectedOrder), len(callOrder), callOrder) } @@ -1710,9 +1713,11 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { } // Test 1: Flag is OFF - first tool variant should be available + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). - WithToolsets([]string{"all"})) + WithToolsets([]string{"all"}). + WithFeatureChecker(checkerOff)) filteredOff := regFlagOff.ForMCPRequest(MCPMethodToolsCall, "get_job_logs") availableOff := filteredOff.AvailableTools(context.Background()) if len(availableOff) != 1 { @@ -1762,11 +1767,13 @@ func TestWithTools_DeprecatedAliasAndFeatureFlag(t *testing.T) { // Test 1: Flag OFF - old_tool should be available via direct name match // (not via alias resolution to new_tool, since old_tool still exists) + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). WithDeprecatedAliases(deprecatedAliases). WithToolsets([]string{}). // No toolsets enabled - WithTools([]string{"old_tool"})) // Explicitly request old tool + WithTools([]string{"old_tool"}). // Explicitly request old tool + WithFeatureChecker(checkerOff)) availableOff := regFlagOff.AvailableTools(context.Background()) if len(availableOff) != 1 { t.Fatalf("Flag OFF: Expected 1 tool, got %d", len(availableOff)) diff --git a/script/print-mcp-diff-configs/main.go b/script/print-mcp-diff-configs/main.go new file mode 100644 index 0000000000..a86bea6ed5 --- /dev/null +++ b/script/print-mcp-diff-configs/main.go @@ -0,0 +1,61 @@ +// Command print-mcp-diff-configs emits the full configuration matrix consumed +// by the mcp-server-diff GitHub Action. The matrix is composed of three parts: +// +// 1. Hand-curated baseline configs (default, read-only, common toolset combos) +// 2. Insiders configs (--insiders, --insiders --read-only) — meta flag that +// expands to the curated insiders feature set +// 3. One config per entry in github.AllowedFeatureFlags — automatically kept +// in sync with the Go source so any new user-controllable feature flag +// gets diffed without touching the workflow +// +// Usage: go run ./script/print-mcp-diff-configs +package main + +import ( + "encoding/json" + "fmt" + "os" + "sort" + + "github.com/github/github-mcp-server/pkg/github" +) + +type config struct { + Name string `json:"name"` + Args string `json:"args"` +} + +func main() { + configs := []config{ + {Name: "default", Args: ""}, + {Name: "read-only", Args: "--read-only"}, + {Name: "toolsets-repos", Args: "--toolsets=repos"}, + {Name: "toolsets-issues", Args: "--toolsets=issues"}, + {Name: "toolsets-context", Args: "--toolsets=context"}, + {Name: "toolsets-pull_requests", Args: "--toolsets=pull_requests"}, + {Name: "toolsets-repos,issues", Args: "--toolsets=repos,issues"}, + {Name: "toolsets-issues,context", Args: "--toolsets=issues,context"}, + {Name: "toolsets-all", Args: "--toolsets=all"}, + {Name: "tools-get_me", Args: "--tools=get_me"}, + {Name: "tools-get_me,list_issues", Args: "--tools=get_me,list_issues"}, + {Name: "toolsets-repos+read-only", Args: "--toolsets=repos --read-only"}, + {Name: "insiders", Args: "--insiders"}, + {Name: "insiders+read-only", Args: "--insiders --read-only"}, + } + + flags := append([]string(nil), github.AllowedFeatureFlags...) + sort.Strings(flags) + for _, f := range flags { + configs = append(configs, config{ + Name: "feature-" + f, + Args: "--features=" + f, + }) + } + + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + if err := enc.Encode(configs); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} From 0c9b8b3bcdf4c191c0f18b368396b6b1cbe2bae7 Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Thu, 21 May 2026 14:55:54 +0100 Subject: [PATCH 4/8] fix: correct MCP features header in cors --- pkg/http/middleware/cors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/http/middleware/cors.go b/pkg/http/middleware/cors.go index fb8e8e548d..b5f4da915b 100644 --- a/pkg/http/middleware/cors.go +++ b/pkg/http/middleware/cors.go @@ -17,7 +17,6 @@ func SetCorsHeaders(h http.Handler) http.Handler { "Mcp-Session-Id", "Mcp-Protocol-Version", "Last-Event-ID", - "X-Custom-Auth-Headers", headers.AuthorizationHeader, headers.MCPReadOnlyHeader, headers.MCPToolsetsHeader, @@ -26,6 +25,7 @@ func SetCorsHeaders(h http.Handler) http.Handler { headers.MCPFeaturesHeader, headers.MCPLockdownHeader, headers.MCPInsidersHeader, + headers.MCPFeaturesHeader, }, ", ") return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From ce1e13de127752263f9d71862941893fa406c08a Mon Sep 17 00:00:00 2001 From: sammorrowdrums Date: Thu, 21 May 2026 16:00:37 +0200 Subject: [PATCH 5/8] docs: regenerate README for CSV output toolset Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/README.md b/README.md index b4a5927b1b..2742d9fafa 100644 --- a/README.md +++ b/README.md @@ -829,6 +829,21 @@ 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) @@ -887,6 +902,22 @@ 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) @@ -897,6 +928,13 @@ 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) @@ -913,6 +951,57 @@ 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) +
@@ -1065,6 +1154,19 @@ 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) @@ -1084,6 +1186,21 @@ 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) @@ -1136,6 +1253,17 @@ 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) @@ -1146,6 +1274,18 @@ 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) @@ -1159,6 +1299,13 @@ 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) @@ -1166,6 +1313,27 @@ 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) +
From 16d49487f95f482d48d54e1058cc7f575223f74e Mon Sep 17 00:00:00 2001 From: RossTarrant Date: Thu, 21 May 2026 15:02:17 +0100 Subject: [PATCH 6/8] fix: remove duplicate MCPFeaturesHeader from CORS headers --- pkg/http/middleware/cors.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/http/middleware/cors.go b/pkg/http/middleware/cors.go index b5f4da915b..2eaf4227b4 100644 --- a/pkg/http/middleware/cors.go +++ b/pkg/http/middleware/cors.go @@ -25,7 +25,6 @@ func SetCorsHeaders(h http.Handler) http.Handler { headers.MCPFeaturesHeader, headers.MCPLockdownHeader, headers.MCPInsidersHeader, - headers.MCPFeaturesHeader, }, ", ") return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From bd8c1601c49fc76fcd9adc7484341fae037a9a8a Mon Sep 17 00:00:00 2001 From: sammorrowdrums Date: Thu, 21 May 2026 16:13:58 +0200 Subject: [PATCH 7/8] ci(mcp-diff): add streamable-http job with header-based configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a sibling mcp-diff-http job that exercises the streamable-http transport against a shared HTTP server, with per-config settings supplied via X-MCP-* request headers — mirroring how the remote server is invoked in production (server-side defaults + per-user header overrides). The config generator gains a -transport flag: - stdio (default, unchanged behaviour) - http-headers (emits headers-only configs targeting a shared server) Two new combined entries layer multiple headers together as a smoke test for header-merging regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/mcp-diff.yml | 48 ++++++ script/print-mcp-diff-configs/main.go | 212 ++++++++++++++++++++++---- 2 files changed, 232 insertions(+), 28 deletions(-) diff --git a/.github/workflows/mcp-diff.yml b/.github/workflows/mcp-diff.yml index b9c7199e56..305428923a 100644 --- a/.github/workflows/mcp-diff.yml +++ b/.github/workflows/mcp-diff.yml @@ -61,3 +61,51 @@ jobs: echo "- New tools/toolsets added" >> $GITHUB_STEP_SUMMARY echo "- Tool descriptions updated" >> $GITHUB_STEP_SUMMARY echo "- Capability changes (intentional improvements)" >> $GITHUB_STEP_SUMMARY + + mcp-diff-http: + runs-on: ubuntu-latest + + steps: + - name: Check out code + uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - name: Build UI + uses: ./.github/actions/build-ui + + - name: Generate diff configurations + id: configs + # See script/print-mcp-diff-configs/main.go. The http-headers variant + # points every config at a shared HTTP server started by the action + # and carries per-config settings via X-MCP-* headers, mirroring how + # the remote server is invoked in production (server-side defaults + + # per-user header overrides). + run: | + { + echo 'configurations<> "$GITHUB_OUTPUT" + + - name: Run MCP Server Diff (streamable-http) + uses: SamMorrowDrums/mcp-server-diff@v2.3.5 + with: + setup_go: "false" + install_command: go mod download + http_start_command: go run ./cmd/github-mcp-server http --port 8082 + http_startup_wait_ms: "5000" + configurations: ${{ steps.configs.outputs.configurations }} + + - name: Add interpretation note + if: always() + run: | + echo "" >> $GITHUB_STEP_SUMMARY + echo "---" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "ℹī¸ **Note:** This job exercises the streamable-http transport against a shared server, with per-config settings supplied via X-MCP-* request headers." >> $GITHUB_STEP_SUMMARY diff --git a/script/print-mcp-diff-configs/main.go b/script/print-mcp-diff-configs/main.go index a86bea6ed5..421c9fce41 100644 --- a/script/print-mcp-diff-configs/main.go +++ b/script/print-mcp-diff-configs/main.go @@ -1,5 +1,5 @@ -// Command print-mcp-diff-configs emits the full configuration matrix consumed -// by the mcp-server-diff GitHub Action. The matrix is composed of three parts: +// Command print-mcp-diff-configs emits the configuration matrix consumed by +// the mcp-server-diff GitHub Action. The matrix is composed of three parts: // // 1. Hand-curated baseline configs (default, read-only, common toolset combos) // 2. Insiders configs (--insiders, --insiders --read-only) — meta flag that @@ -8,54 +8,210 @@ // in sync with the Go source so any new user-controllable feature flag // gets diffed without touching the workflow // -// Usage: go run ./script/print-mcp-diff-configs +// The same logical matrix is rendered for two transports, selected by +// -transport: +// +// stdio Default. Args are appended to the action's top-level +// +// start_command (one stdio process per config). +// +// http-headers streamable-http transport against a shared HTTP server. The +// +// server is started once with no extra flags and every config +// provides its settings via X-MCP-* request headers, mirroring +// how the remote server is invoked in production (server-side +// defaults + per-user header overrides). +// +// Usage: +// +// go run ./script/print-mcp-diff-configs +// go run ./script/print-mcp-diff-configs -transport http-headers package main import ( "encoding/json" + "flag" "fmt" "os" "sort" + "strings" "github.com/github/github-mcp-server/pkg/github" + mcphdr "github.com/github/github-mcp-server/pkg/http/headers" ) type config struct { - Name string `json:"name"` - Args string `json:"args"` + Name string `json:"name"` + Args string `json:"args,omitempty"` + Transport string `json:"transport,omitempty"` + ServerURL string `json:"server_url,omitempty"` + Headers map[string]string `json:"headers,omitempty"` } +// baseEntry describes one logical configuration in transport-agnostic form. +// settings are translated to either CLI flags or X-MCP-* headers depending on +// the target transport. +type baseEntry struct { + name string + settings settings +} + +type settings struct { + toolsets string // comma-separated, "" for defaults + tools string + excludeTools string + features string + readOnly bool + insiders bool + lockdown bool +} + +const httpServerURL = "http://localhost:8082/mcp" + func main() { - configs := []config{ - {Name: "default", Args: ""}, - {Name: "read-only", Args: "--read-only"}, - {Name: "toolsets-repos", Args: "--toolsets=repos"}, - {Name: "toolsets-issues", Args: "--toolsets=issues"}, - {Name: "toolsets-context", Args: "--toolsets=context"}, - {Name: "toolsets-pull_requests", Args: "--toolsets=pull_requests"}, - {Name: "toolsets-repos,issues", Args: "--toolsets=repos,issues"}, - {Name: "toolsets-issues,context", Args: "--toolsets=issues,context"}, - {Name: "toolsets-all", Args: "--toolsets=all"}, - {Name: "tools-get_me", Args: "--tools=get_me"}, - {Name: "tools-get_me,list_issues", Args: "--tools=get_me,list_issues"}, - {Name: "toolsets-repos+read-only", Args: "--toolsets=repos --read-only"}, - {Name: "insiders", Args: "--insiders"}, - {Name: "insiders+read-only", Args: "--insiders --read-only"}, + transport := flag.String("transport", "stdio", "Transport to target: stdio or http-headers") + flag.Parse() + + entries := baseEntries() + + var out []config + switch *transport { + case "stdio": + for _, e := range entries { + out = append(out, config{Name: e.name, Args: e.settings.toArgs()}) + } + case "http-headers": + for _, e := range entries { + h := e.settings.toHeaders() + if h == nil { + h = map[string]string{} + } + // The action's top-level headers may be replaced (not merged) by + // per-config headers, so always include the bearer token here. + // The token must match a recognized GitHub prefix so the server's + // Authorization parser accepts it without contacting the API. + h[mcphdr.AuthorizationHeader] = "Bearer ghp_test" + out = append(out, config{ + Name: e.name, + Transport: "streamable-http", + ServerURL: httpServerURL, + Headers: h, + }) + } + default: + fmt.Fprintf(os.Stderr, "unknown transport %q (want stdio or http-headers)\n", *transport) + os.Exit(2) + } + + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + if err := enc.Encode(out); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} + +func baseEntries() []baseEntry { + entries := []baseEntry{ + {name: "default"}, + {name: "read-only", settings: settings{readOnly: true}}, + {name: "toolsets-repos", settings: settings{toolsets: "repos"}}, + {name: "toolsets-issues", settings: settings{toolsets: "issues"}}, + {name: "toolsets-context", settings: settings{toolsets: "context"}}, + {name: "toolsets-pull_requests", settings: settings{toolsets: "pull_requests"}}, + {name: "toolsets-repos,issues", settings: settings{toolsets: "repos,issues"}}, + {name: "toolsets-issues,context", settings: settings{toolsets: "issues,context"}}, + {name: "toolsets-all", settings: settings{toolsets: "all"}}, + {name: "tools-get_me", settings: settings{tools: "get_me"}}, + {name: "tools-get_me,list_issues", settings: settings{tools: "get_me,list_issues"}}, + {name: "toolsets-repos+read-only", settings: settings{toolsets: "repos", readOnly: true}}, + {name: "insiders", settings: settings{insiders: true}}, + {name: "insiders+read-only", settings: settings{insiders: true, readOnly: true}}, + // Combined entries: exercise multiple settings together so we catch + // regressions when several X-MCP-* headers (or CLI flags) are merged. + {name: "combined-toolsets+exclude+readonly", settings: settings{ + toolsets: "repos,issues", + excludeTools: "delete_file", + readOnly: true, + }}, + {name: "combined-insiders+toolsets+features", settings: settings{ + insiders: true, + toolsets: "repos", + features: firstFeatureFlag(), + }}, } flags := append([]string(nil), github.AllowedFeatureFlags...) sort.Strings(flags) for _, f := range flags { - configs = append(configs, config{ - Name: "feature-" + f, - Args: "--features=" + f, + entries = append(entries, baseEntry{ + name: "feature-" + f, + settings: settings{features: f}, }) } + return entries +} - enc := json.NewEncoder(os.Stdout) - enc.SetIndent("", " ") - if err := enc.Encode(configs); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) +func (s settings) toArgs() string { + var parts []string + if s.toolsets != "" { + parts = append(parts, "--toolsets="+s.toolsets) + } + if s.tools != "" { + parts = append(parts, "--tools="+s.tools) + } + if s.excludeTools != "" { + parts = append(parts, "--exclude-tools="+s.excludeTools) + } + if s.features != "" { + parts = append(parts, "--features="+s.features) + } + if s.readOnly { + parts = append(parts, "--read-only") + } + if s.insiders { + parts = append(parts, "--insiders") + } + if s.lockdown { + parts = append(parts, "--lockdown-mode") + } + return strings.Join(parts, " ") +} + +func (s settings) toHeaders() map[string]string { + h := map[string]string{} + if s.toolsets != "" { + h[mcphdr.MCPToolsetsHeader] = s.toolsets } + if s.tools != "" { + h[mcphdr.MCPToolsHeader] = s.tools + } + if s.excludeTools != "" { + h[mcphdr.MCPExcludeToolsHeader] = s.excludeTools + } + if s.features != "" { + h[mcphdr.MCPFeaturesHeader] = s.features + } + if s.readOnly { + h[mcphdr.MCPReadOnlyHeader] = "true" + } + if s.insiders { + h[mcphdr.MCPInsidersHeader] = "true" + } + if s.lockdown { + h[mcphdr.MCPLockdownHeader] = "true" + } + if len(h) == 0 { + return nil + } + return h +} + +func firstFeatureFlag() string { + flags := append([]string(nil), github.AllowedFeatureFlags...) + if len(flags) == 0 { + return "" + } + sort.Strings(flags) + return flags[0] } From 8088e60c9387e43bfbf389336ae332759ca1e861 Mon Sep 17 00:00:00 2001 From: sammorrowdrums Date: Thu, 21 May 2026 16:44:52 +0200 Subject: [PATCH 8/8] docs: regenerate after merging main Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/README.md b/README.md index 516fb3bfdf..6d29649658 100644 --- a/README.md +++ b/README.md @@ -829,6 +829,21 @@ 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) @@ -894,6 +909,22 @@ 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) @@ -904,6 +935,13 @@ 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) @@ -920,6 +958,57 @@ 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) +
@@ -1072,6 +1161,19 @@ 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) @@ -1091,6 +1193,21 @@ 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) @@ -1143,6 +1260,17 @@ 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) @@ -1153,6 +1281,18 @@ 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) @@ -1166,6 +1306,13 @@ 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) @@ -1173,6 +1320,27 @@ 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) +