From 9c25c022a9fc76c9aa81117ced73fe4efdb156ba Mon Sep 17 00:00:00 2001 From: SamMorrowDrums Date: Thu, 21 May 2026 14:31:04 +0200 Subject: [PATCH 1/8] refactor: generic toolset+name sort, clarify feature flag intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- pkg/github/feature_flags.go | 9 ++++-- pkg/inventory/filters.go | 57 ++++++++++++++++++++++++------------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 9adfa38d2..87a43d022 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -38,8 +38,13 @@ 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 — InsidersFeatureFlags may +// intentionally include flags that are not user-controllable. // // 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/inventory/filters.go b/pkg/inventory/filters.go index 20a157de6..891de23fa 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -103,15 +103,26 @@ func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *Ser 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 { + if toolsetID(items[i]) != toolsetID(items[j]) { + return toolsetID(items[i]) < toolsetID(items[j]) } - 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. @@ -132,6 +143,12 @@ func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool { // AvailableToolsWithoutFeatureFiltering returns tools that pass every filter // except FeatureFlagEnable/FeatureFlagDisable. +// +// HTTP mode uses this to build the static (process-wide) inventory, which acts +// as an upper bound on what any request may see. Per-request feature flags +// (from headers like X-MCP-Features or X-MCP-Insiders) are evaluated later +// when the per-request inventory is derived, so feature-flagged variants must +// be preserved here. func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) []ServerTool { var result []ServerTool for i := range r.tools { @@ -147,12 +164,10 @@ func (r *Inventory) AvailableToolsWithoutFeatureFiltering(ctx context.Context) [ } 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, @@ -178,7 +193,9 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso } // AvailableResourceTemplatesWithoutFeatureFiltering returns resource templates -// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. +// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. See +// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in +// HTTP mode. func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context.Context) []ServerResourceTemplate { var result []ServerResourceTemplate for i := range r.resourceTemplates { @@ -194,12 +211,10 @@ func (r *Inventory) AvailableResourceTemplatesWithoutFeatureFiltering(_ context. } 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, @@ -224,8 +239,10 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { return result } -// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every filter -// except FeatureFlagEnable/FeatureFlagDisable. +// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every +// filter except FeatureFlagEnable/FeatureFlagDisable. See +// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in +// HTTP mode. func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt { var result []ServerPrompt for i := range r.prompts { From ae002a5a22c59efbb24490a659dc12ab84a9ad2d Mon Sep 17 00:00:00 2001 From: SamMorrowDrums Date: Thu, 21 May 2026 14:32:21 +0200 Subject: [PATCH 2/8] 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> --- pkg/github/feature_flags.go | 9 +++++++-- pkg/github/feature_flags_test.go | 12 ++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 87a43d022..19399e7ac 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -43,8 +43,13 @@ type FeatureFlags struct { // 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 — InsidersFeatureFlags may -// intentionally include flags that are not user-controllable. +// 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 9bc1be473..9f31ada38 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}, From 644741deecf28a22910edacc46d31a3542897064 Mon Sep 17 00:00:00 2001 From: SamMorrowDrums Date: Thu, 21 May 2026 14:37:30 +0200 Subject: [PATCH 3/8] refactor(inventory): collapse three *WithoutFeatureFiltering helpers into StaticUpperBound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- pkg/http/handler.go | 2 +- pkg/http/handler_test.go | 2 +- pkg/inventory/filters.go | 81 +++++++++++++++------------------------- 3 files changed, 33 insertions(+), 52 deletions(-) diff --git a/pkg/http/handler.go b/pkg/http/handler.go index dd6161d52..bcf4709e1 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -357,7 +357,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun // (/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.StaticUpperBound(ctx) } // InventoryFiltersForRequest applies filters to the inventory builder diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index dfb402093..972d5cbeb 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -779,7 +779,7 @@ func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTo } ctx := context.Background() - return inv.AvailableToolsWithoutFeatureFiltering(ctx), inv.AvailableResourceTemplatesWithoutFeatureFiltering(ctx), inv.AvailablePromptsWithoutFeatureFiltering(ctx) + return inv.StaticUpperBound(ctx) } func TestCrossOriginProtection(t *testing.T) { diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 891de23fa..5aef5fcc4 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -141,28 +141,6 @@ func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool { return result } -// AvailableToolsWithoutFeatureFiltering returns tools that pass every filter -// except FeatureFlagEnable/FeatureFlagDisable. -// -// HTTP mode uses this to build the static (process-wide) inventory, which acts -// as an upper bound on what any request may see. Per-request feature flags -// (from headers like X-MCP-Features or X-MCP-Insiders) are evaluated later -// when the per-request inventory is derived, so feature-flagged variants must -// be preserved here. -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) { sortByToolsetThenName(resourceTemplates, func(r ServerResourceTemplate) ToolsetID { return r.Toolset.ID }, @@ -192,24 +170,6 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso return result } -// AvailableResourceTemplatesWithoutFeatureFiltering returns resource templates -// that pass every filter except FeatureFlagEnable/FeatureFlagDisable. See -// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in -// HTTP mode. -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) { sortByToolsetThenName(prompts, func(p ServerPrompt) ToolsetID { return p.Toolset.ID }, @@ -239,22 +199,43 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { return result } -// AvailablePromptsWithoutFeatureFiltering returns prompts that pass every -// filter except FeatureFlagEnable/FeatureFlagDisable. See -// AvailableToolsWithoutFeatureFiltering for why feature flags are deferred in -// HTTP mode. -func (r *Inventory) AvailablePromptsWithoutFeatureFiltering(_ context.Context) []ServerPrompt { - var result []ServerPrompt +// StaticUpperBound returns the tools, resource templates, and prompts that pass +// every current filter except FeatureFlagEnable/FeatureFlagDisable. It exists +// for HTTP mode, which builds a process-wide static inventory at startup as an +// upper bound on what any request may expose. Feature flags from request +// headers (X-MCP-Features, X-MCP-Insiders) cannot be evaluated yet at that +// point, so feature-gated variants — including the dual jsonOnly/csvCapable +// pattern used for CSV output — must all be carried through to the per-request +// inventory, which then resolves the flag. +func (r *Inventory) StaticUpperBound(ctx context.Context) ([]ServerTool, []ServerResourceTemplate, []ServerPrompt) { + var tools []ServerTool + for i := range r.tools { + tool := &r.tools[i] + if r.isToolEnabledWithFeatureFlags(ctx, tool, false) { + tools = append(tools, *tool) + } + } + sortTools(tools) + + var resources []ServerResourceTemplate + for i := range r.resourceTemplates { + res := &r.resourceTemplates[i] + if r.isToolsetEnabled(res.Toolset.ID) { + resources = append(resources, *res) + } + } + sortResourceTemplates(resources) + + var prompts []ServerPrompt for i := range r.prompts { prompt := &r.prompts[i] if r.isToolsetEnabled(prompt.Toolset.ID) { - result = append(result, *prompt) + prompts = append(prompts, *prompt) } } + sortPrompts(prompts) - sortPrompts(result) - - return result + return tools, resources, prompts } // filterToolsByName returns tools matching the given name, checking deprecated aliases. From 11912cc01b5ba0f9f226214dcb41cb6686f57832 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 14:50:22 +0200 Subject: [PATCH 4/8] 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> --- pkg/github/csv_output.go | 30 ++++---- pkg/github/csv_output_test.go | 128 ++++++++++++++++----------------- pkg/github/tools.go | 2 +- pkg/http/handler.go | 20 +++--- pkg/http/handler_test.go | 9 ++- pkg/inventory/builder.go | 5 +- pkg/inventory/filters.go | 58 ++++----------- pkg/inventory/registry_test.go | 38 +++++----- 8 files changed, 130 insertions(+), 160 deletions(-) diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index 5c06a1c8c..5adffd176 100644 --- a/pkg/github/csv_output.go +++ b/pkg/github/csv_output.go @@ -42,24 +42,19 @@ 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 — so a single tool with +// a runtime check is sufficient (no dual feature-gated variants needed). +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 +70,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 80ff8a789..d0bef3893 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/tools.go b/pkg/github/tools.go index e7530b672..6496c85f8 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 bcf4709e1..e585a8656 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.StaticUpperBound(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 972d5cbeb..74e28a6e4 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.StaticUpperBound(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 2642c6127..192c7af4a 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -128,7 +128,10 @@ 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. +// If checker is nil, feature flag filtering is skipped entirely — the inventory +// then returns the full upper bound, including dual-name variants. The +// per-request inventory must always install a checker so MCP registration +// (which only serves one tool per name) sees a deduplicated set. // Returns self for chaining. func (b *Builder) WithFeatureChecker(checker FeatureFlagChecker) *Builder { b.featureChecker = checker diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 5aef5fcc4..cf0a50844 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -36,9 +36,18 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool } // 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 +// - If no feature checker is configured, feature flags are not evaluated and +// the item passes through. This gives the unfiltered upper bound used by +// HTTP mode at startup, before per-request feature checkers exist. The +// per-request inventory always installs a checker, so any dual-name +// variants are resolved before MCP registration (which can only serve a +// given tool name once). +// - 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 { + if r.featureChecker == nil { + return true + } // Check enable flag - item requires this flag to be on if enableFlag != "" && !r.checkFeatureFlag(ctx, enableFlag) { return false @@ -58,10 +67,6 @@ 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) @@ -74,7 +79,7 @@ func (r *Inventory) isToolEnabledWithFeatureFlags(ctx context.Context, tool *Ser } } // 2. Check feature flags - if checkFeatureFlags && !r.isFeatureFlagAllowed(ctx, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { + if !r.isFeatureFlagAllowed(ctx, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { return false } // 3. Check read-only filter (applies to all tools) @@ -199,45 +204,6 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { return result } -// StaticUpperBound returns the tools, resource templates, and prompts that pass -// every current filter except FeatureFlagEnable/FeatureFlagDisable. It exists -// for HTTP mode, which builds a process-wide static inventory at startup as an -// upper bound on what any request may expose. Feature flags from request -// headers (X-MCP-Features, X-MCP-Insiders) cannot be evaluated yet at that -// point, so feature-gated variants — including the dual jsonOnly/csvCapable -// pattern used for CSV output — must all be carried through to the per-request -// inventory, which then resolves the flag. -func (r *Inventory) StaticUpperBound(ctx context.Context) ([]ServerTool, []ServerResourceTemplate, []ServerPrompt) { - var tools []ServerTool - for i := range r.tools { - tool := &r.tools[i] - if r.isToolEnabledWithFeatureFlags(ctx, tool, false) { - tools = append(tools, *tool) - } - } - sortTools(tools) - - var resources []ServerResourceTemplate - for i := range r.resourceTemplates { - res := &r.resourceTemplates[i] - if r.isToolsetEnabled(res.Toolset.ID) { - resources = append(resources, *res) - } - } - sortResourceTemplates(resources) - - var prompts []ServerPrompt - for i := range r.prompts { - prompt := &r.prompts[i] - if r.isToolsetEnabled(prompt.Toolset.ID) { - prompts = append(prompts, *prompt) - } - } - sortPrompts(prompts) - - return tools, resources, prompts -} - // 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 8e35861f1..c62666d6d 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") @@ -1710,9 +1712,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 +1766,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)) From ee04e6acfdcc990be954dd618f72fc4e66c574fc Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 14:53:10 +0200 Subject: [PATCH 5/8] 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> --- .github/workflows/mcp-diff.yml | 35 ++++++++------- script/print-mcp-diff-configs/main.go | 61 +++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 16 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 bb6341c09..b9c7199e5 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/script/print-mcp-diff-configs/main.go b/script/print-mcp-diff-configs/main.go new file mode 100644 index 000000000..a86bea6ed --- /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 48f08a893829e077eb3c165beaefc9f3f34ad049 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 14:54:55 +0200 Subject: [PATCH 6/8] 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> --- docs/insiders-features.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 59147c4f5..90afe7219 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`. From be14b888751e2f0785caf48b7a88276dcda555d5 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 15:07:19 +0200 Subject: [PATCH 7/8] 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> --- pkg/inventory/builder.go | 31 +++++++++--- pkg/inventory/filters.go | 86 +++++++++++++++++++++------------- pkg/inventory/registry_test.go | 13 ++--- 3 files changed, 85 insertions(+), 45 deletions(-) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 192c7af4a..9ecaca1f5 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -127,11 +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, feature flag filtering is skipped entirely — the inventory -// then returns the full upper bound, including dual-name variants. The -// per-request inventory must always install a checker so MCP registration -// (which only serves one tool per name) sees a deduplicated set. +// 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 @@ -203,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, @@ -210,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 cf0a50844..7624ea6b3 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -35,37 +35,57 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool return enabled } -// isFeatureFlagAllowed checks if an item passes feature flag filtering. -// - If no feature checker is configured, feature flags are not evaluated and -// the item passes through. This gives the unfiltered upper bound used by -// HTTP mode at startup, before per-request feature checkers exist. The -// per-request inventory always installs a checker, so any dual-name -// variants are resolved before MCP registration (which can only serve a -// given tool name once). +// 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 (r *Inventory) isFeatureFlagAllowed(ctx context.Context, enableFlag, disableFlag string) bool { - if r.featureChecker == nil { - return true - } - // Check enable flag - item requires this flag to be on - if enableFlag != "" && !r.checkFeatureFlag(ctx, enableFlag) { - return false +func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag, disableFlag string) bool { + if enableFlag != "" { + enabled, err := checker(ctx, enableFlag) + if err != nil { + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", enableFlag, err) + return false + } + if !enabled { + return false + } } - // Check disable flag - item is excluded if this flag is on - if disableFlag != "" && r.checkFeatureFlag(ctx, disableFlag) { - return false + if disableFlag != "" { + enabled, err := checker(ctx, disableFlag) + if err != nil { + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", disableFlag, err) + return false + } + if enabled { + 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 { // 1. Check tool's own Enabled function first if tool.Enabled != nil { @@ -78,15 +98,11 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { return false } } - // 2. Check feature flags - if !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 { @@ -97,11 +113,11 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { 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 } @@ -160,8 +176,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) { @@ -189,8 +208,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) { diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index c62666d6d..75de9c574 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1652,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{} @@ -1688,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) } From 0a21966565adf2477307bfbe6c6fa7c88b3623cd Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 21 May 2026 15:33:04 +0200 Subject: [PATCH 8/8] 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> --- pkg/github/csv_output.go | 3 +-- pkg/inventory/filters.go | 33 ++++++++++++++++----------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index 5adffd176..cb70e32d7 100644 --- a/pkg/github/csv_output.go +++ b/pkg/github/csv_output.go @@ -45,8 +45,7 @@ type csvOutputDocument struct { // 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 — so a single tool with -// a runtime check is sufficient (no dual feature-gated variants needed). +// unchanged — only the response payload format differs. func withCSVOutput(tools []inventory.ServerTool) []inventory.ServerTool { for i := range tools { if !isCSVOutputTool(tools[i]) { diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 7624ea6b3..e2effd8ca 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -44,25 +44,23 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool // - 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 { - if enableFlag != "" { - enabled, err := checker(ctx, enableFlag) + // 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", enableFlag, err) - return false - } - if !enabled { + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flag, err) return false } + return enabled } - if disableFlag != "" { - enabled, err := checker(ctx, disableFlag) - if err != nil { - fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", disableFlag, err) - return false - } - if enabled { - return false - } + if enableFlag != "" && !check(enableFlag) { + return false + } + if disableFlag != "" && check(disableFlag) { + return false } return true } @@ -130,8 +128,9 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { // prompts). func sortByToolsetThenName[T any](items []T, toolsetID func(T) ToolsetID, name func(T) string) { sort.Slice(items, func(i, j int) bool { - if toolsetID(items[i]) != toolsetID(items[j]) { - return toolsetID(items[i]) < toolsetID(items[j]) + idI, idJ := toolsetID(items[i]), toolsetID(items[j]) + if idI != idJ { + return idI < idJ } return name(items[i]) < name(items[j]) })