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/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`. diff --git a/pkg/github/csv_output.go b/pkg/github/csv_output.go index 5c06a1c8c..cb70e32d7 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 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/feature_flags.go b/pkg/github/feature_flags.go index 9adfa38d2..19399e7ac 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 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}, 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 dd6161d52..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.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 dfb402093..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.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 2642c6127..9ecaca1f5 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 20a157de6..e2effd8ca 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 8e35861f1..75de9c574 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 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) + } +}