From e5eba30267931aef06dcf055446d936abcfda4ca Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 27 Aug 2026 11:49:35 +0200 Subject: [PATCH 1/2] refactor(features)!: use functional availability rules Resolve declared inventory features once per request and share the request-owned cache with in-handler feature checks. BREAKING CHANGE: Inventory items now use FeatureRule instead of FeatureFlagEnable, FeatureFlagEnableAll, and FeatureFlagDisable; FeatureFlagChecker now accepts FeatureFlag. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- cmd/github-mcp-server/feature_flag_docs.go | 10 +- cmd/github-mcp-server/generate_docs.go | 7 +- docs/feature-flags.md | 31 +++- docs/insiders-features.md | 12 +- internal/ghmcp/server.go | 6 +- pkg/github/actions_test.go | 2 +- pkg/github/context_tools_test.go | 3 +- pkg/github/csv_output_test.go | 12 +- pkg/github/dependencies.go | 33 +--- pkg/github/dependencies_test.go | 7 +- pkg/github/feature_flags.go | 70 ++++++-- pkg/github/feature_flags_test.go | 68 ++++---- pkg/github/find_duplicate.go | 2 +- pkg/github/find_duplicate_test.go | 5 +- pkg/github/granular_tools_test.go | 14 +- pkg/github/issue_dependencies.go | 4 +- pkg/github/issue_dependencies_test.go | 9 +- pkg/github/issues.go | 4 +- pkg/github/issues_granular.go | 24 +-- pkg/github/issues_test.go | 2 +- pkg/github/pullrequests.go | 26 ++- pkg/github/pullrequests_granular.go | 42 +++-- pkg/github/repositories.go | 2 +- pkg/github/repositories_test.go | 2 +- pkg/github/server.go | 2 +- pkg/github/server_test.go | 10 +- pkg/github/tools.go | 10 +- pkg/github/tools_validation_test.go | 8 +- pkg/github/ui_capability_test.go | 9 +- pkg/github/ui_tools.go | 2 +- pkg/github/ui_tools_test.go | 3 +- pkg/http/handler.go | 3 + pkg/http/handler_test.go | 25 ++- pkg/http/server.go | 4 +- pkg/http/server_test.go | 42 +++-- pkg/inventory/builder.go | 23 +-- pkg/inventory/features.go | 189 +++++++++++++++++++++ pkg/inventory/features_test.go | 95 +++++++++++ pkg/inventory/filters.go | 106 +++--------- pkg/inventory/prompts.go | 8 +- pkg/inventory/registry.go | 60 ++++++- pkg/inventory/registry_test.go | 101 ++++++----- pkg/inventory/resources.go | 8 +- pkg/inventory/server_tool.go | 14 +- script/print-mcp-diff-configs/main.go | 4 +- 45 files changed, 747 insertions(+), 376 deletions(-) create mode 100644 pkg/inventory/features.go create mode 100644 pkg/inventory/features_test.go diff --git a/cmd/github-mcp-server/feature_flag_docs.go b/cmd/github-mcp-server/feature_flag_docs.go index e52237b138..a0ecea9d2a 100644 --- a/cmd/github-mcp-server/feature_flag_docs.go +++ b/cmd/github-mcp-server/feature_flag_docs.go @@ -33,7 +33,7 @@ func generateFeatureFlagsDocs(docsPath string) error { // whose registration or definition differs from the default user experience. // Each affected tool is printed with its full schema using the same writer // used by the README so the output style stays consistent. -func generateFlaggedToolsDoc(flags []string, emptyMessage string) string { +func generateFlaggedToolsDoc(flags []inventory.FeatureFlag, emptyMessage string) string { t, _ := translations.TranslationHelper() defaultTools := indexToolsByName(buildInventoryWithFlags(t, nil).ToolsForRegistration(context.Background())) @@ -73,8 +73,8 @@ func generateFlaggedToolsDoc(flags []string, emptyMessage string) string { // differs from the default-flagged inventory when only the given flag is on, // plus tools that exist only in the flag-on inventory. Results are sorted by // tool name. -func flaggedToolDiff(t translations.TranslationHelperFunc, flag string, defaultTools map[string]inventory.ServerTool) []inventory.ServerTool { - flagTools := buildInventoryWithFlags(t, map[string]bool{flag: true}).ToolsForRegistration(context.Background()) +func flaggedToolDiff(t translations.TranslationHelperFunc, flag inventory.FeatureFlag, defaultTools map[string]inventory.ServerTool) []inventory.ServerTool { + flagTools := buildInventoryWithFlags(t, map[inventory.FeatureFlag]bool{flag: true}).ToolsForRegistration(context.Background()) out := make([]inventory.ServerTool, 0) seen := make(map[string]struct{}, len(flagTools)) @@ -99,8 +99,8 @@ func flaggedToolDiff(t translations.TranslationHelperFunc, flag string, defaultT // buildInventoryWithFlags constructs an inventory whose feature checker treats // the given flags as enabled and every other flag as disabled. Passing nil // produces the default-flagged inventory. -func buildInventoryWithFlags(t translations.TranslationHelperFunc, enabled map[string]bool) *inventory.Inventory { - checker := func(_ context.Context, flag string) (bool, error) { +func buildInventoryWithFlags(t translations.TranslationHelperFunc, enabled map[inventory.FeatureFlag]bool) *inventory.Inventory { + checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) { return enabled[flag], nil } inv, _ := github.NewInventory(t). diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 791a40b886..0d360ac1f6 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -31,7 +31,7 @@ func init() { // noFeatureFlagsChecker reports every feature flag as disabled. It models the // default user experience used by the generated documentation. -func noFeatureFlagsChecker(_ context.Context, _ string) (bool, error) { +func noFeatureFlagsChecker(_ context.Context, _ inventory.FeatureFlag) (bool, error) { return false, nil } @@ -61,9 +61,8 @@ func generateReadmeDocs(readmePath string) error { // The README documents the default user experience: tools that are // enabled with no special flags set. Installing a checker that reports - // every flag as disabled excludes tools gated by FeatureFlagEnable and - // keeps the legacy variants of tools gated by FeatureFlagDisable, so - // flag-gated duplicates don't appear twice. + // every flag as disabled keeps the default variants selected by functional + // feature rules, so flag-gated duplicates don't appear twice. // Build() can only fail if WithTools specifies invalid tools - not used here r, _ := github.NewInventory(t). WithToolsets([]string{"all"}). diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0ed3f9dc0e..fc868db968 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -20,15 +20,34 @@ Only flags listed in [`AllowedFeatureFlags`](../pkg/github/feature_flags.go) can be enabled by end users. Insiders-only flags are not user-toggleable. +## Declaring tool availability + +Tools, resources, and prompts use `inventory.NewFeatureRule` when feature flags +change whether they are available. Each rule declares the flags it references +and evaluates them with a fail-closed `FeatureResolver`, so normal Go boolean +expressions can represent AND, OR, NOT, and mixed conditions: + +```go +tool.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{x, y}, + func(featureAsBool inventory.FeatureResolver) bool { + return !(featureAsBool(x) && featureAsBool(y)) + }, +) +``` + +The service deduplicates the declared flags, resolves each one at most once for +the request, and shares those values with tool dependencies. Feature checks +inside handlers continue to use `deps.IsFeatureEnabled`. + --- ## Tools affected by each flag -The list below is regenerated from the Go source. For each user-controllable -feature flag, it lists every tool whose **inventory or input schema** differs -from the default — either because the flag introduces a new tool, or because -it selects a flag-aware variant of an existing tool. Flags that only affect -runtime behavior (such as output formatting) won't appear here. +The list below is regenerated by comparing the default tool surface with each +user-controllable flag enabled individually. Complex multi-flag rules may +require separate documentation. Flags that only affect runtime behavior (such +as output formatting) won't appear here. @@ -357,7 +376,7 @@ runtime behavior (such as output formatting) won't appear here. ### `thread_resolution_reason` - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews - - **Required OAuth Scopes**: `repo` + - **OAuth Challenge Scopes**: `repo` - `body`: Review comment text (string, optional) - `commitID`: SHA of commit to review (string, optional) - `event`: Review action to perform. (string, optional) diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 6c941d97a1..9702b31bb9 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -205,6 +205,13 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for 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. +For tool availability, each functional feature rule statically declares the +flags it reads. The service deduplicates those declarations, resolves every +relevant flag once into request-owned state, and then evaluates all rules as +in-memory boolean expressions. The same state backs +`deps.IsFeatureEnabled`, so checks made inside a tool call reuse resolved values +and lazily cache any handler-only flag using the live tool-call context. + `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. @@ -216,5 +223,6 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for 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`. +4. For tool availability, attach an `inventory.NewFeatureRule` that declares every flag used by its predicate. For behavior inside a handler, use `deps.IsFeatureEnabled(ctx, FeatureFlagX)`. +5. Gate on concrete flags, never on `cfg.InsidersMode`. There is a `TestGitHubPackageDoesNotReadInsidersMode` guard test that fails if `pkg/github` reads `InsidersMode` directly. +6. The MCP-diff CI workflow picks up new entries in `AllowedFeatureFlags` automatically — see `.github/workflows/mcp-diff.yml`. diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index dadc05744b..02a6fb6038 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -256,7 +256,7 @@ type StdioServerConfig struct { EnabledTools []string // EnabledFeatures is a list of feature flags that are enabled - // Items with FeatureFlagEnable matching an entry in this list will be available + // Tool feature rules evaluate entries in this list. EnabledFeatures []string // ReadOnly indicates if we should only register read-only tools @@ -435,8 +435,8 @@ func RunStdioServer(cfg StdioServerConfig) error { // using the centralized ResolveFeatureFlags function. For the local server, // features are resolved once at startup from --features CLI flag and insiders mode. func createFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { - featureSet := github.ResolveFeatureFlags(enabledFeatures, insidersMode) - return func(_ context.Context, flagName string) (bool, error) { + featureSet := github.ResolveFeatureFlags(github.FeatureFlagsFromStrings(enabledFeatures), insidersMode) + return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { return featureSet[flagName], nil } } diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 964bc95a6b..f390c31771 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -574,7 +574,7 @@ func Test_ActionsGetJobLogs(t *testing.T) { // Note: consolidated ActionsGetJobLogs has same tool name "get_job_logs" as the individual tool // but with different descriptions. We skip toolsnap validation here since the individual // tool's toolsnap already exists and is tested in Test_GetJobLogs. - // The consolidated tool has FeatureFlagEnable set, so only one will be active at a time. + // The functional feature rules ensure only one variant is active at a time. assert.Equal(t, "get_job_logs", toolDef.Tool.Name) assert.NotEmpty(t, toolDef.Tool.Description) inputSchema := toolDef.Tool.InputSchema.(*jsonschema.Schema) diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 0825158abb..7ee3e16551 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -9,6 +9,7 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -189,7 +190,7 @@ func Test_GetMe_IFC_FeatureFlag(t *testing.T) { translations.NullTranslationHelper, FeatureFlags{}, 0, - func(_ context.Context, flagName string) (bool, error) { + func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { return flagName == FeatureFlagIFCLabels && enabled, nil }, stubExporters(), diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go index 5cc6fe7e54..f9bb0e28c5 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -29,20 +29,18 @@ func TestCSVOutputAppliedToDefaultListTools(t *testing.T) { require.Len(t, available, 2) listing := requireToolByName(t, available, "list_things") - assert.Empty(t, listing.FeatureFlagEnable) - assert.Empty(t, listing.FeatureFlagDisable) + assert.True(t, listing.FeatureRule.IsZero()) getting := requireToolByName(t, available, "get_thing") - assert.Empty(t, getting.FeatureFlagEnable) - assert.Empty(t, getting.FeatureFlagDisable) + assert.True(t, getting.FeatureRule.IsZero()) } } func TestCSVOutputAppliesToFlagGatedListTools(t *testing.T) { enabledOnly := testCSVOutputTool("list_things", `[{"number":1}]`) - enabledOnly.FeatureFlagEnable = FeatureFlagFileBlame + enabledOnly.FeatureRule = featureEnabledRule(FeatureFlagFileBlame) disabledOnly := testCSVOutputTool("list_legacy_things", `[{"number":2}]`) - disabledOnly.FeatureFlagDisable = []string{FeatureFlagFileBlame} + disabledOnly.FeatureRule = featureDisabledRule(FeatureFlagFileBlame) tools := withCSVOutput([]inventory.ServerTool{enabledOnly, disabledOnly}) require.Len(t, tools, 2) @@ -368,7 +366,7 @@ type csvOutputTestDeps struct { csvOn bool } -func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag string) bool { +func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag inventory.FeatureFlag) bool { return flag == FeatureFlagCSVOutput && d.csvOn } diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 0de3e3b279..9152e073a2 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -6,7 +6,6 @@ import ( "fmt" "log/slog" "net/http" - "os" ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/http/transport" @@ -95,7 +94,7 @@ type ToolDependencies interface { GetContentWindowSize() int // IsFeatureEnabled checks if a feature flag is enabled. - IsFeatureEnabled(ctx context.Context, flagName string) bool + IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool // Logger returns the structured logger, optionally enriched with // request-scoped data from ctx. Integrators provide their own slog.Handler @@ -207,19 +206,8 @@ func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSea // IsFeatureEnabled checks if a feature flag is enabled. // Returns false if the feature checker is nil, flag name is empty, or an error occurs. // This allows tools to conditionally change behavior based on feature flags. -func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { - if d.featureChecker == nil || flagName == "" { - return false - } - - enabled, err := d.featureChecker(ctx, flagName) - if err != nil { - // Log error but don't fail the tool - treat as disabled - fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err) - return false - } - - return enabled +func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { + return inventory.ResolveFeature(ctx, d.featureChecker, flag) } // NewTool creates a ServerTool that retrieves ToolDependencies from context at call time. @@ -496,17 +484,6 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics { } // IsFeatureEnabled checks if a feature flag is enabled. -func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flagName string) bool { - if d.featureChecker == nil || flagName == "" { - return false - } - - enabled, err := d.featureChecker(ctx, flagName) - if err != nil { - // Log error but don't fail the tool - treat as disabled - fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err) - return false - } - - return enabled +func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { + return inventory.ResolveFeature(ctx, d.featureChecker, flag) } diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index 223fc8aca4..c434921f58 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -14,6 +14,7 @@ import ( ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/headers" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/observability" "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/translations" @@ -202,7 +203,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() // Create a feature checker that returns true for "test_flag" - checker := func(_ context.Context, flagName string) (bool, error) { + checker := func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { return flagName == "test_flag", nil } @@ -253,7 +254,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { t.Parallel() // Create a feature checker - checker := func(_ context.Context, _ string) (bool, error) { + checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) { return true, nil } @@ -388,7 +389,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) { t.Parallel() // Create a feature checker that returns an error - checker := func(_ context.Context, _ string) (bool, error) { + checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) { return false, errors.New("checker error") } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 27202c5c83..e8219075f8 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -1,47 +1,51 @@ package github -import "slices" +import ( + "slices" + + "github.com/github/github-mcp-server/pkg/inventory" +) // MCPAppsFeatureFlag is the feature flag name for MCP Apps (interactive UI forms). -const MCPAppsFeatureFlag = "remote_mcp_ui_apps" +const MCPAppsFeatureFlag inventory.FeatureFlag = "remote_mcp_ui_apps" // MCPAppsDisableFormDeferralFeatureFlag disables handing write-tool calls off // to MCP App forms while preserving MCP Apps UI metadata and result views. -const MCPAppsDisableFormDeferralFeatureFlag = "mcp_apps_disable_form_deferral" +const MCPAppsDisableFormDeferralFeatureFlag inventory.FeatureFlag = "mcp_apps_disable_form_deferral" // FeatureFlagCSVOutput is the feature flag name for CSV output on list tools. -const FeatureFlagCSVOutput = "csv_output" +const FeatureFlagCSVOutput inventory.FeatureFlag = "csv_output" // FeatureFlagIFCLabels is the feature flag name for IFC security labels in tool results. -const FeatureFlagIFCLabels = "ifc_labels" +const FeatureFlagIFCLabels inventory.FeatureFlag = "ifc_labels" // FeatureFlagFileBlame is the feature flag name for the get_file_blame tool, // which exposes git blame information for a file. It is gated so the extra tool // is not advertised by default, keeping the tool surface small unless opted in. -const FeatureFlagFileBlame = "file_blame" +const FeatureFlagFileBlame inventory.FeatureFlag = "file_blame" // FeatureFlagIssueDependencies is the feature flag name for the issue dependency // tools (issue_dependency_read / issue_dependency_write), which read and edit an // issue's blocked-by / blocking relationships. It is gated so these tools are not // advertised in the default surface, keeping the fixed tool-schema cost small // unless explicitly opted in. -const FeatureFlagIssueDependencies = "issue_dependencies" +const FeatureFlagIssueDependencies inventory.FeatureFlag = "issue_dependencies" // FeatureFlagDuplicateDetection is the feature flag name for the find_duplicate // tool, which returns ranked duplicate candidates for an existing issue. It is // gated so the extra tool is not advertised by default, and is deliberately // excluded from insiders mode so duplicate detection is only ever an explicit // opt-in. -const FeatureFlagDuplicateDetection = "duplicate_detection" +const FeatureFlagDuplicateDetection inventory.FeatureFlag = "duplicate_detection" // FeatureFlagThreadResolutionReason exposes resolution reasons for Copilot review threads. -const FeatureFlagThreadResolutionReason = "thread_resolution_reason" +const FeatureFlagThreadResolutionReason inventory.FeatureFlag = "thread_resolution_reason" // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. // This is the single source of truth for which flags are user-controllable. -var AllowedFeatureFlags = []string{ +var AllowedFeatureFlags = []inventory.FeatureFlag{ MCPAppsFeatureFlag, MCPAppsDisableFormDeferralFeatureFlag, FeatureFlagCSVOutput, @@ -58,7 +62,7 @@ var AllowedFeatureFlags = []string{ // When insiders mode is active, all flags in this list are treated as enabled. // This is the single source of truth for what "insiders" means in terms of // feature flag expansion. -var InsidersFeatureFlags = []string{ +var InsidersFeatureFlags = []inventory.FeatureFlag{ MCPAppsFeatureFlag, FeatureFlagCSVOutput, FeatureFlagFileBlame, @@ -70,6 +74,40 @@ type FeatureFlags struct { LockdownMode bool } +func featureEnabledRule(feature inventory.FeatureFlag) inventory.FeatureRule { + return inventory.NewFeatureRule( + []inventory.FeatureFlag{feature}, + func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(feature) + }, + ) +} + +func featureDisabledRule(feature inventory.FeatureFlag) inventory.FeatureRule { + return inventory.NewFeatureRule( + []inventory.FeatureFlag{feature}, + func(featureAsBool inventory.FeatureResolver) bool { + return !featureAsBool(feature) + }, + ) +} + +var ( + issuesGranularFeatureRule = featureEnabledRule(FeatureFlagIssuesGranular) + issuesConsolidatedFeatureRule = featureDisabledRule(FeatureFlagIssuesGranular) + pullRequestsGranularFeatureRule = featureEnabledRule(FeatureFlagPullRequestsGranular) + pullRequestsConsolidatedRule = featureDisabledRule(FeatureFlagPullRequestsGranular) +) + +// FeatureFlagsFromStrings converts feature names from transport or CLI input. +func FeatureFlagsFromStrings(features []string) []inventory.FeatureFlag { + result := make([]inventory.FeatureFlag, len(features)) + for i, feature := range features { + result[i] = inventory.FeatureFlag(feature) + } + return result +} + // ResolveFeatureFlags computes the effective set of enabled feature flags by: // 1. Taking the user-supplied flags (from --features or X-MCP-Features) and // keeping only those present in AllowedFeatureFlags. Unknown or unsafe @@ -85,11 +123,11 @@ type FeatureFlags struct { // 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 { - effective := make(map[string]bool) - for _, f := range enabledFeatures { - if slices.Contains(AllowedFeatureFlags, f) { - effective[f] = true +func ResolveFeatureFlags(enabledFeatures []inventory.FeatureFlag, insidersMode bool) map[inventory.FeatureFlag]bool { + effective := make(map[inventory.FeatureFlag]bool) + for _, feature := range enabledFeatures { + if slices.Contains(AllowedFeatureFlags, feature) { + effective[feature] = true } } if insidersMode { diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index dafacfa79a..1b01e750a4 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -17,14 +17,14 @@ import ( ) // RemoteMCPEnthusiasticGreeting is a dummy test feature flag . -const RemoteMCPEnthusiasticGreeting = "remote_mcp_enthusiastic_greeting" +const RemoteMCPEnthusiasticGreeting inventory.FeatureFlag = "remote_mcp_enthusiastic_greeting" -func featureCheckerFor(enabledFlags ...string) func(context.Context, string) (bool, error) { - enabled := make(map[string]bool, len(enabledFlags)) +func featureCheckerFor(enabledFlags ...inventory.FeatureFlag) inventory.FeatureFlagChecker { + enabled := make(map[inventory.FeatureFlag]bool, len(enabledFlags)) for _, flag := range enabledFlags { enabled[flag] = true } - return func(_ context.Context, flagName string) (bool, error) { + return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { return enabled[flagName], nil } } @@ -91,7 +91,7 @@ func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - var enabledFlags []string + var enabledFlags []inventory.FeatureFlag if tt.featureFlagEnabled { enabledFlags = append(enabledFlags, RemoteMCPEnthusiasticGreeting) } @@ -140,26 +140,26 @@ func TestResolveFeatureFlags(t *testing.T) { tests := []struct { name string - enabledFeatures []string + enabledFeatures []inventory.FeatureFlag insidersMode bool - expectedFlags []string - unexpectedFlags []string + expectedFlags []inventory.FeatureFlag + unexpectedFlags []inventory.FeatureFlag }{ { name: "no features, no insiders", enabledFeatures: nil, expectedFlags: nil, - unexpectedFlags: []string{MCPAppsFeatureFlag}, + unexpectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, }, { name: "explicit feature enabled", - enabledFeatures: []string{MCPAppsFeatureFlag}, - expectedFlags: []string{MCPAppsFeatureFlag}, + enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag}, + expectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, }, { name: "MCP Apps form deferral can be disabled directly", - enabledFeatures: []string{MCPAppsDisableFormDeferralFeatureFlag}, - expectedFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, + enabledFeatures: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, + expectedFlags: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, }, { name: "insiders mode enables insiders flags", @@ -171,50 +171,50 @@ func TestResolveFeatureFlags(t *testing.T) { name: "insiders mode does not auto-enable ifc labels", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []string{FeatureFlagIFCLabels}, + unexpectedFlags: []inventory.FeatureFlag{FeatureFlagIFCLabels}, }, { name: "insiders mode does not disable MCP Apps form deferral", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, + unexpectedFlags: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, }, { name: "ifc_labels can be directly enabled", - enabledFeatures: []string{FeatureFlagIFCLabels}, - expectedFlags: []string{FeatureFlagIFCLabels}, + enabledFeatures: []inventory.FeatureFlag{FeatureFlagIFCLabels}, + expectedFlags: []inventory.FeatureFlag{FeatureFlagIFCLabels}, }, { name: "unknown flags are filtered out", - enabledFeatures: []string{"unknown_flag", "another_unknown"}, - unexpectedFlags: []string{"unknown_flag", "another_unknown"}, + enabledFeatures: []inventory.FeatureFlag{"unknown_flag", "another_unknown"}, + unexpectedFlags: []inventory.FeatureFlag{"unknown_flag", "another_unknown"}, }, { name: "mix of known and unknown flags", - enabledFeatures: []string{MCPAppsFeatureFlag, "unknown_flag"}, - expectedFlags: []string{MCPAppsFeatureFlag}, - unexpectedFlags: []string{"unknown_flag"}, + enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag, "unknown_flag"}, + expectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, + unexpectedFlags: []inventory.FeatureFlag{"unknown_flag"}, }, { name: "user-only flags can be enabled but are not turned on by insiders", - enabledFeatures: []string{FeatureFlagIssuesGranular}, + enabledFeatures: []inventory.FeatureFlag{FeatureFlagIssuesGranular}, insidersMode: false, - expectedFlags: []string{FeatureFlagIssuesGranular}, + expectedFlags: []inventory.FeatureFlag{FeatureFlagIssuesGranular}, }, { name: "thread resolution reason can be directly enabled", - enabledFeatures: []string{FeatureFlagThreadResolutionReason}, - expectedFlags: []string{FeatureFlagThreadResolutionReason}, + enabledFeatures: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, + expectedFlags: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, }, { name: "insiders does not enable user-only allowed flags", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []string{FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular}, + unexpectedFlags: []inventory.FeatureFlag{FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular}, }, { name: "explicit plus insiders deduplicates", - enabledFeatures: []string{MCPAppsFeatureFlag}, + enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag}, insidersMode: true, expectedFlags: InsidersFeatureFlags, }, @@ -237,7 +237,7 @@ func TestResolveFeatureFlags(t *testing.T) { func TestThreadResolutionReasonToolVariants(t *testing.T) { tests := []struct { name string - flags []string + flags []inventory.FeatureFlag host utils.HostType toolName string hasReason bool @@ -248,30 +248,30 @@ func TestThreadResolutionReasonToolVariants(t *testing.T) { }, { name: "consolidated flag on", - flags: []string{FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, toolName: "pull_request_review_write", hasReason: true, }, { name: "granular flag off", - flags: []string{FeatureFlagPullRequestsGranular}, + flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular}, toolName: "resolve_review_thread", }, { name: "granular flag on", - flags: []string{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, toolName: "resolve_review_thread", hasReason: true, }, { name: "consolidated flag on GHES", - flags: []string{FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, host: utils.HostTypeGHES, toolName: "pull_request_review_write", }, { name: "granular flag on GHES", - flags: []string{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, host: utils.HostTypeGHES, toolName: "resolve_review_thread", }, diff --git a/pkg/github/find_duplicate.go b/pkg/github/find_duplicate.go index 65cb6f2002..236f53a489 100644 --- a/pkg/github/find_duplicate.go +++ b/pkg/github/find_duplicate.go @@ -178,6 +178,6 @@ func FindDuplicate(t translations.TranslationHelperFunc) inventory.ServerTool { result = attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, result, ifc.LabelRepoUserContent) return result, nil, nil }) - st.FeatureFlagEnable = FeatureFlagDuplicateDetection + st.FeatureRule = featureEnabledRule(FeatureFlagDuplicateDetection) return st } diff --git a/pkg/github/find_duplicate_test.go b/pkg/github/find_duplicate_test.go index 9e20d958b4..b2c032989b 100644 --- a/pkg/github/find_duplicate_test.go +++ b/pkg/github/find_duplicate_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/jsonschema-go/jsonschema" "github.com/stretchr/testify/assert" @@ -20,8 +21,8 @@ func Test_FindDuplicate(t *testing.T) { // Verify tool definition once (flag-gated variant snap). serverTool := FindDuplicate(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagDuplicateDetection, tool)) - require.Equal(t, FeatureFlagDuplicateDetection, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+string(FeatureFlagDuplicateDetection), tool)) + require.Equal(t, []inventory.FeatureFlag{FeatureFlagDuplicateDetection}, serverTool.FeatureRule.Features()) assert.Equal(t, "find_duplicate", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 5ef0c0a662..1fe943bfea 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -20,10 +20,16 @@ import ( "github.com/stretchr/testify/require" ) -func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag string) []inventory.ServerTool { +func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag inventory.FeatureFlag) []inventory.ServerTool { var result []inventory.ServerTool for _, tool := range AllTools(translations.NullTranslationHelper) { - if tool.Toolset.ID == toolsetID && tool.FeatureFlagEnable == featureFlag && len(tool.FeatureFlagEnableAll) == 0 { + features := tool.FeatureRule.Features() + usesFeature := false + for _, feature := range features { + usesFeature = usesFeature || feature == featureFlag + } + if tool.Toolset.ID == toolsetID && usesFeature && + tool.FeatureRule.Enabled(func(flag inventory.FeatureFlag) bool { return flag == featureFlag }) { result = append(result, tool) } } @@ -102,7 +108,7 @@ func TestIssuesGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) { - assert.Equal(t, FeatureFlagIssuesGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Equal(t, []inventory.FeatureFlag{FeatureFlagIssuesGranular}, tool.FeatureRule.Features(), "tool %s", tool.Tool.Name) } }) } @@ -138,7 +144,7 @@ func TestPullRequestsGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) { - assert.Equal(t, FeatureFlagPullRequestsGranular, tool.FeatureFlagEnable, "tool %s", tool.Tool.Name) + assert.Contains(t, tool.FeatureRule.Features(), FeatureFlagPullRequestsGranular, "tool %s", tool.Tool.Name) } }) } diff --git a/pkg/github/issue_dependencies.go b/pkg/github/issue_dependencies.go index 246192736e..dc78c9eccc 100644 --- a/pkg/github/issue_dependencies.go +++ b/pkg/github/issue_dependencies.go @@ -103,7 +103,7 @@ Options are: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagEnable = FeatureFlagIssueDependencies + st.FeatureRule = featureEnabledRule(FeatureFlagIssueDependencies) return st } @@ -321,7 +321,7 @@ Options are: result, err := writeIssueDependency(ctx, client, method, blocked, blocking) return result, nil, err }) - st.FeatureFlagEnable = FeatureFlagIssueDependencies + st.FeatureRule = featureEnabledRule(FeatureFlagIssueDependencies) return st } diff --git a/pkg/github/issue_dependencies_test.go b/pkg/github/issue_dependencies_test.go index 6af9c504ed..35baa815ee 100644 --- a/pkg/github/issue_dependencies_test.go +++ b/pkg/github/issue_dependencies_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/jsonschema-go/jsonschema" "github.com/stretchr/testify/assert" @@ -34,8 +35,8 @@ func Test_IssueDependencyRead(t *testing.T) { // Verify tool definition once (flag-gated variant snap) serverTool := IssueDependencyRead(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueDependencies, tool)) - require.Equal(t, FeatureFlagIssueDependencies, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+string(FeatureFlagIssueDependencies), tool)) + require.Equal(t, []inventory.FeatureFlag{FeatureFlagIssueDependencies}, serverTool.FeatureRule.Features()) assert.Equal(t, "issue_dependency_read", tool.Name) assert.NotEmpty(t, tool.Description) @@ -189,8 +190,8 @@ func Test_IssueDependencyWrite(t *testing.T) { // Verify tool definition once (flag-gated variant snap) serverTool := IssueDependencyWrite(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueDependencies, tool)) - require.Equal(t, FeatureFlagIssueDependencies, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+string(FeatureFlagIssueDependencies), tool)) + require.Equal(t, []inventory.FeatureFlag{FeatureFlagIssueDependencies}, serverTool.FeatureRule.Features()) assert.Equal(t, "issue_dependency_write", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 9b6ee5da6b..28bf8d7813 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1679,7 +1679,7 @@ func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + st.FeatureRule = issuesConsolidatedFeatureRule return st } @@ -2700,7 +2700,7 @@ Options are: return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil } }) - st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + st.FeatureRule = issuesConsolidatedFeatureRule return st } diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index 0b1cdd0e7c..c6544495c6 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -108,7 +108,7 @@ func issueUpdateTool( return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -234,7 +234,7 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -460,7 +460,7 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -678,7 +678,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -866,7 +866,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1064,7 +1064,7 @@ func GranularUpdateIssueState(t translations.TranslationHelperFunc) inventory.Se return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1138,7 +1138,7 @@ func GranularAddSubIssue(t translations.TranslationHelperFunc) inventory.ServerT return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1207,7 +1207,7 @@ func GranularRemoveSubIssue(t translations.TranslationHelperFunc) inventory.Serv return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1292,7 +1292,7 @@ func GranularReprioritizeSubIssue(t translations.TranslationHelperFunc) inventor return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1580,7 +1580,7 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1662,7 +1662,7 @@ func GranularAddIssueReaction(t translations.TranslationHelperFunc) inventory.Se return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } @@ -1744,6 +1744,6 @@ func GranularAddIssueCommentReaction(t translations.TranslationHelperFunc) inven return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagIssuesGranular + st.FeatureRule = issuesGranularFeatureRule return st } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index bf024b545a..6ca5b4b101 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1833,7 +1833,7 @@ func Test_CreateIssue(t *testing.T) { serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Empty(t, serverTool.FeatureFlagEnable) + require.Equal(t, []inventory.FeatureFlag{FeatureFlagIssuesGranular}, serverTool.FeatureRule.Features()) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index afd2778510..6a92d50a5a 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1166,7 +1166,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultText(string(r)), nil, nil }) - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + st.FeatureRule = pullRequestsConsolidatedRule return st } @@ -1895,12 +1895,24 @@ Available methods: } }) if withResolutionReason { - st.FeatureFlagEnable = FeatureFlagThreadResolutionReason - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + st.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, FeatureFlagPullRequestsGranular}, + func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(FeatureFlagThreadResolutionReason) && + !featureAsBool(FeatureFlagPullRequestsGranular) + }, + ) } else { - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} - if cfg.hostType != utils.HostTypeGHES { - st.FeatureFlagDisable = append(st.FeatureFlagDisable, FeatureFlagThreadResolutionReason) + if cfg.hostType == utils.HostTypeGHES { + st.FeatureRule = pullRequestsConsolidatedRule + } else { + st.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, FeatureFlagPullRequestsGranular}, + func(featureAsBool inventory.FeatureResolver) bool { + return !featureAsBool(FeatureFlagThreadResolutionReason) && + !featureAsBool(FeatureFlagPullRequestsGranular) + }, + ) } } return st @@ -2447,7 +2459,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S }) return result, nil, err }) - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + st.FeatureRule = pullRequestsConsolidatedRule return st } diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index 3a4b9fc810..e1d245a951 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -103,7 +103,7 @@ func prUpdateTool( return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -272,7 +272,7 @@ func GranularUpdatePullRequestDraftState(t translations.TranslationHelperFunc) i return utils.NewToolResultText("pull request marked as ready for review"), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -351,7 +351,7 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -436,7 +436,7 @@ func GranularCreatePullRequestReview(t translations.TranslationHelperFunc) inven return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -500,7 +500,7 @@ func GranularSubmitPendingPullRequestReview(t translations.TranslationHelperFunc return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -555,7 +555,7 @@ func GranularDeletePendingPullRequestReview(t translations.TranslationHelperFunc return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -666,7 +666,7 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -748,11 +748,25 @@ func granularResolveReviewThread(t translations.TranslationHelperFunc, withResol return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular - if withResolutionReason { - st.FeatureFlagEnableAll = []string{FeatureFlagThreadResolutionReason} - } else if cfg.hostType != utils.HostTypeGHES { - st.FeatureFlagDisable = []string{FeatureFlagThreadResolutionReason} + switch { + case withResolutionReason: + st.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(FeatureFlagPullRequestsGranular) && + featureAsBool(FeatureFlagThreadResolutionReason) + }, + ) + case cfg.hostType == utils.HostTypeGHES: + st.FeatureRule = pullRequestsGranularFeatureRule + default: + st.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(FeatureFlagPullRequestsGranular) && + !featureAsBool(FeatureFlagThreadResolutionReason) + }, + ) } return st } @@ -797,7 +811,7 @@ func GranularUnresolveReviewThread(t translations.TranslationHelperFunc) invento return result, nil, err }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } @@ -879,6 +893,6 @@ func GranularAddPullRequestReviewCommentReaction(t translations.TranslationHelpe return utils.NewToolResultText(string(r)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagPullRequestsGranular + st.FeatureRule = pullRequestsGranularFeatureRule return st } diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 8575d994cc..3ef7876802 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -3027,7 +3027,7 @@ func GetFileBlame(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultText(string(payload)), nil, nil }, ) - st.FeatureFlagEnable = FeatureFlagFileBlame + st.FeatureRule = featureEnabledRule(FeatureFlagFileBlame) return st } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index a1ea5ff340..94e7f7b076 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -5889,7 +5889,7 @@ func Test_GetFileBlame(t *testing.T) { // get_file_blame is gated so it is not advertised unless the feature flag // (or insiders mode) opts it in. - assert.Equal(t, FeatureFlagFileBlame, serverTool.FeatureFlagEnable, "get_file_blame must be gated behind the file_blame feature flag") + assert.Equal(t, []inventory.FeatureFlag{FeatureFlagFileBlame}, serverTool.FeatureRule.Features()) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") diff --git a/pkg/github/server.go b/pkg/github/server.go index b8f0197889..8335ce0fea 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -35,7 +35,7 @@ type MCPServerConfig struct { EnabledTools []string // EnabledFeatures is a list of feature flags that are enabled - // Items with FeatureFlagEnable matching an entry in this list will be available + // Tool feature rules evaluate entries in this list. EnabledFeatures []string // ReadOnly indicates if we should only offer read-only tools diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 07cb63c85f..43037bf893 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -62,10 +62,12 @@ func (s stubDeps) GetRawClient(ctx context.Context) (*raw.Client, error) { func (s stubDeps) GetRepoAccessCache(_ context.Context) (*lockdown.RepoAccessCache, error) { return s.repoAccessCache, nil } -func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t } -func (s stubDeps) GetFlags(_ context.Context) FeatureFlags { return s.flags } -func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } -func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } +func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t } +func (s stubDeps) GetFlags(_ context.Context) FeatureFlags { return s.flags } +func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } +func (s stubDeps) IsFeatureEnabled(_ context.Context, _ inventory.FeatureFlag) bool { + return false +} func (s stubDeps) Logger(_ context.Context) *slog.Logger { return s.obsv.Logger() } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 8d568878db..8176b2f98e 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -155,15 +155,19 @@ var ( // Feature flag names for granular tool variants. // When active, consolidated tools are replaced by single-purpose granular tools. - FeatureFlagIssuesGranular = "issues_granular" - FeatureFlagPullRequestsGranular = "pull_requests_granular" + FeatureFlagIssuesGranular inventory.FeatureFlag = "issues_granular" + FeatureFlagPullRequestsGranular inventory.FeatureFlag = "pull_requests_granular" ) // HeaderAllowedFeatureFlags returns the feature flags that clients may enable via // the X-MCP-Features header. It delegates to AllowedFeatureFlags as the single // source of truth. func HeaderAllowedFeatureFlags() []string { - return slices.Clone(AllowedFeatureFlags) + flags := make([]string, len(AllowedFeatureFlags)) + for i, flag := range AllowedFeatureFlags { + flags[i] = string(flag) + } + return flags } var ( diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index dac04b5d54..bbc98558ab 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -140,7 +140,7 @@ func TestNoDuplicateToolNames(t *testing.T) { // First pass: identify tools that have feature flags (mutually exclusive at runtime) for _, tool := range tools { - if tool.FeatureFlagEnable != "" || len(tool.FeatureFlagDisable) > 0 { + if !tool.FeatureRule.IsZero() { featureFlagged[tool.Tool.Name] = true } } @@ -156,6 +156,12 @@ func TestNoDuplicateToolNames(t *testing.T) { } } +func TestMCPAppsFeatureFlagMatchesInventory(t *testing.T) { + inv, err := NewInventory(stubTranslation).Build() + require.NoError(t, err) + assert.Contains(t, inv.RequiredFeatures(), MCPAppsFeatureFlag) +} + // TestNoDuplicateResourceNames ensures all resources have unique names func TestNoDuplicateResourceNames(t *testing.T) { resources := AllResources(stubTranslation) diff --git a/pkg/github/ui_capability_test.go b/pkg/github/ui_capability_test.go index 1c49ee15be..812d6fde89 100644 --- a/pkg/github/ui_capability_test.go +++ b/pkg/github/ui_capability_test.go @@ -5,6 +5,7 @@ import ( "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -95,17 +96,17 @@ func Test_shouldDeferToForm_featureFlags(t *testing.T) { tests := []struct { name string - enabledFlags []string + enabledFlags []inventory.FeatureFlag want bool }{ { name: "MCP Apps enabled defers to form", - enabledFlags: []string{MCPAppsFeatureFlag}, + enabledFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, want: true, }, { name: "form deferral disabled executes directly", - enabledFlags: []string{ + enabledFlags: []inventory.FeatureFlag{ MCPAppsFeatureFlag, MCPAppsDisableFormDeferralFeatureFlag, }, @@ -113,7 +114,7 @@ func Test_shouldDeferToForm_featureFlags(t *testing.T) { }, { name: "form deferral opt-out does not enable MCP Apps", - enabledFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, + enabledFlags: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, want: false, }, { diff --git a/pkg/github/ui_tools.go b/pkg/github/ui_tools.go index 62bba06ef6..b8d6cd31a5 100644 --- a/pkg/github/ui_tools.go +++ b/pkg/github/ui_tools.go @@ -98,7 +98,7 @@ func UIGet(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } }) - st.FeatureFlagEnable = MCPAppsFeatureFlag + st.FeatureRule = featureEnabledRule(MCPAppsFeatureFlag) return st } diff --git a/pkg/github/ui_tools_test.go b/pkg/github/ui_tools_test.go index 4a4981875b..d400752bdf 100644 --- a/pkg/github/ui_tools_test.go +++ b/pkg/github/ui_tools_test.go @@ -12,6 +12,7 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" "github.com/google/jsonschema-go/jsonschema" @@ -105,7 +106,7 @@ func Test_UIGet(t *testing.T) { assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "repo") assert.ElementsMatch(t, tool.InputSchema.(*jsonschema.Schema).Required, []string{"method", "owner"}) assert.True(t, tool.Annotations.ReadOnlyHint, "ui_get should be read-only") - assert.Equal(t, MCPAppsFeatureFlag, serverTool.FeatureFlagEnable, "ui_get should be gated on the MCP Apps feature flag") + assert.Equal(t, []inventory.FeatureFlag{MCPAppsFeatureFlag}, serverTool.FeatureRule.Features()) // ui_get must be app-only so the host hides it from the agent's tool list // while keeping it callable by the views (MCP Apps 2026-01-26 spec). diff --git a/pkg/http/handler.go b/pkg/http/handler.go index e4a9d198ec..f8c71e3d26 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -40,6 +40,7 @@ type Handler struct { inventoryFactoryFunc InventoryFactoryFunc oauthCfg *oauth.Config scopeFetcher scopes.FetcherInterface + featureChecker inventory.FeatureFlagChecker schemaCache *mcp.SchemaCache } @@ -126,6 +127,7 @@ func NewHTTPMcpHandler( inventoryFactoryFunc: inventoryFactory, oauthCfg: opts.OAuthConfig, scopeFetcher: scopeFetcher, + featureChecker: opts.FeatureChecker, schemaCache: schemaCache, } } @@ -214,6 +216,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if methodInfo, ok := ghcontext.MCPMethod(r.Context()); ok && methodInfo != nil { invToUse = inv.ForMCPRequest(methodInfo.Method, methodInfo.ItemName) } + r = r.WithContext(inventory.WithResolvedFeatures(r.Context(), h.featureChecker, invToUse.RequiredFeatures())) ghServer, err := h.githubMcpServerFactory(r, h.deps, invToUse, &github.MCPServerConfig{ Version: h.config.Version, diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index f051084785..b1aa7b0b79 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -60,12 +60,19 @@ func (f allScopesFetcher) FetchTokenScopes(_ context.Context, _ string) ([]strin var _ scopes.FetcherInterface = allScopesFetcher{} -func mockToolWithFeatureFlag(name, toolsetID string, readOnly bool, enableFlag, disableFlag string) inventory.ServerTool { +func mockToolWithFeatureFlag(name, toolsetID string, readOnly bool, enableFlag, disableFlag inventory.FeatureFlag) inventory.ServerTool { tool := mockTool(name, toolsetID, readOnly) - tool.FeatureFlagEnable = enableFlag + features := make([]inventory.FeatureFlag, 0, 2) + if enableFlag != "" { + features = append(features, enableFlag) + } if disableFlag != "" { - tool.FeatureFlagDisable = []string{disableFlag} + features = append(features, disableFlag) } + tool.FeatureRule = inventory.NewFeatureRule(features, func(featureAsBool inventory.FeatureResolver) bool { + return (enableFlag == "" || featureAsBool(enableFlag)) && + (disableFlag == "" || !featureAsBool(disableFlag)) + }) return tool } @@ -348,8 +355,8 @@ func TestHTTPHandlerRoutes(t *testing.T) { // Create feature checker that reads from context without whitelist validation // (the whitelist is tested separately; here we test the filtering logic) - featureChecker := func(ctx context.Context, flag string) (bool, error) { - return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil + featureChecker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { + return slices.Contains(ghcontext.GetHeaderFeatures(ctx), string(flag)), nil } apiHost, err := utils.NewAPIHost("https://api.github.com") @@ -553,8 +560,8 @@ func TestStaticConfigEnforcement(t *testing.T) { var capturedInventory *inventory.Inventory var capturedCtx context.Context - featureChecker := func(ctx context.Context, flag string) (bool, error) { - return slices.Contains(ghcontext.GetHeaderFeatures(ctx), flag), nil + featureChecker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { + return slices.Contains(ghcontext.GetHeaderFeatures(ctx), string(flag)), nil } apiHost, err := utils.NewAPIHost("https://api.github.com") @@ -737,7 +744,9 @@ func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) { available := inv.AvailableTools(ctx) require.Len(t, available, 1) assert.Equal(t, "list_issues", available[0].Tool.Name) - assert.Equal(t, github.FeatureFlagCSVOutput, available[0].FeatureFlagEnable) + assert.True(t, available[0].FeatureRule.Enabled(func(flag inventory.FeatureFlag) bool { + return flag == github.FeatureFlagCSVOutput + })) } func TestStaticInventoryDisablesOnlyDeleteRepository(t *testing.T) { diff --git a/pkg/http/server.go b/pkg/http/server.go index cc2d23d3ac..60f509953e 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -312,13 +312,13 @@ func initGlobalToolScopeMap(t translations.TranslationHelperFunc, hostType utils // createHTTPFeatureChecker creates a feature checker that resolves static CLI // features plus per-request header features and insiders mode. func createHTTPFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { - return func(ctx context.Context, flag string) (bool, error) { + return func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { headerFeatures := ghcontext.GetHeaderFeatures(ctx) features := make([]string, 0, len(enabledFeatures)+len(headerFeatures)) features = append(features, enabledFeatures...) features = append(features, headerFeatures...) - effective := github.ResolveFeatureFlags(features, insidersMode || ghcontext.IsInsidersMode(ctx)) + effective := github.ResolveFeatureFlags(github.FeatureFlagsFromStrings(features), insidersMode || ghcontext.IsInsidersMode(ctx)) return effective[flag], nil } } diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index a8c4e1a90b..8a94f9c4f9 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -312,41 +312,41 @@ func TestInitGlobalToolScopeMapUsesHost(t *testing.T) { func TestCreateHTTPFeatureChecker(t *testing.T) { tests := []struct { name string - staticFeatures []string + staticFeatures []inventory.FeatureFlag staticInsiders bool - flagName string - headerFeatures []string + flagName inventory.FeatureFlag + headerFeatures []inventory.FeatureFlag insidersMode bool wantEnabled bool }{ { name: "allowed issues_granular flag accepted from header", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []string{github.FeatureFlagIssuesGranular}, + headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular}, wantEnabled: true, }, { name: "allowed pull_requests_granular flag accepted from header", flagName: github.FeatureFlagPullRequestsGranular, - headerFeatures: []string{github.FeatureFlagPullRequestsGranular}, + headerFeatures: []inventory.FeatureFlag{github.FeatureFlagPullRequestsGranular}, wantEnabled: true, }, { name: "MCP Apps flag accepted from header", flagName: github.MCPAppsFeatureFlag, - headerFeatures: []string{github.MCPAppsFeatureFlag}, + headerFeatures: []inventory.FeatureFlag{github.MCPAppsFeatureFlag}, wantEnabled: true, }, { name: "MCP Apps form deferral opt-out accepted from header", flagName: github.MCPAppsDisableFormDeferralFeatureFlag, - headerFeatures: []string{github.MCPAppsDisableFormDeferralFeatureFlag}, + headerFeatures: []inventory.FeatureFlag{github.MCPAppsDisableFormDeferralFeatureFlag}, wantEnabled: true, }, { name: "unknown flag in header is ignored", flagName: "unknown_flag", - headerFeatures: []string{"unknown_flag"}, + headerFeatures: []inventory.FeatureFlag{"unknown_flag"}, wantEnabled: false, }, { @@ -358,19 +358,19 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { { name: "allowed flag with different flag in header returns false", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []string{github.FeatureFlagPullRequestsGranular}, + headerFeatures: []inventory.FeatureFlag{github.FeatureFlagPullRequestsGranular}, wantEnabled: false, }, { name: "multiple allowed flags in header", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []string{github.FeatureFlagIssuesGranular, github.FeatureFlagPullRequestsGranular}, + headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular, github.FeatureFlagPullRequestsGranular}, wantEnabled: true, }, { name: "empty header features", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []string{}, + headerFeatures: []inventory.FeatureFlag{}, wantEnabled: false, }, { @@ -387,15 +387,15 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { }, { name: "static feature is enabled without header", - staticFeatures: []string{github.FeatureFlagCSVOutput}, + staticFeatures: []inventory.FeatureFlag{github.FeatureFlagCSVOutput}, flagName: github.FeatureFlagCSVOutput, wantEnabled: true, }, { name: "static features combine with header features", - staticFeatures: []string{github.FeatureFlagCSVOutput}, + staticFeatures: []inventory.FeatureFlag{github.FeatureFlagCSVOutput}, flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []string{github.FeatureFlagIssuesGranular}, + headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular}, wantEnabled: true, }, { @@ -420,10 +420,10 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - checker := createHTTPFeatureChecker(tt.staticFeatures, tt.staticInsiders) + checker := createHTTPFeatureChecker(featureFlagStrings(tt.staticFeatures), tt.staticInsiders) ctx := context.Background() if len(tt.headerFeatures) > 0 { - ctx = ghcontext.WithHeaderFeatures(ctx, tt.headerFeatures) + ctx = ghcontext.WithHeaderFeatures(ctx, featureFlagStrings(tt.headerFeatures)) } if tt.insidersMode { ctx = ghcontext.WithInsidersMode(ctx, true) @@ -514,7 +514,15 @@ func TestConfigureRequestState(t *testing.T) { func TestHeaderAllowedFeatureFlagsMatchesAllowed(t *testing.T) { // Ensure HeaderAllowedFeatureFlags delegates to AllowedFeatureFlags allowed := github.HeaderAllowedFeatureFlags() - assert.Equal(t, github.AllowedFeatureFlags, allowed, + assert.Equal(t, featureFlagStrings(github.AllowedFeatureFlags), allowed, "HeaderAllowedFeatureFlags() should match AllowedFeatureFlags") assert.NotEmpty(t, allowed, "AllowedFeatureFlags should not be empty") } + +func featureFlagStrings(flags []inventory.FeatureFlag) []string { + result := make([]string, len(flags)) + for i, flag := range flags { + result[i] = string(flag) + } + return result +} diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 9ecaca1f57..20cd9f5838 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -17,7 +17,7 @@ var ( // mcpAppsFeatureFlag is the feature flag name that controls MCP Apps UI metadata. // This is defined here to avoid importing pkg/github (which imports pkg/inventory). // The value must match github.MCPAppsFeatureFlag. -const mcpAppsFeatureFlag = "remote_mcp_ui_apps" +const mcpAppsFeatureFlag FeatureFlag = "remote_mcp_ui_apps" // ToolFilter is a function that determines if a tool should be included. // Returns true if the tool should be included, false to exclude it. @@ -125,15 +125,10 @@ func (b *Builder) WithTools(toolNames []string) *Builder { return b } -// WithFeatureChecker sets the feature flag checker function. -// The checker receives a context (for actor extraction) and feature flag name, -// 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. +// WithFeatureChecker sets the feature flag checker function. Inventory items +// declare their feature dependencies and functional availability rules through +// FeatureRule. Checks are deduplicated into request-owned resolution state; +// errors are logged and treated as disabled. // // When the checker is nil, no feature-flag filter is installed; tools, // resources, and prompts pass through feature-flag gating unchanged. The @@ -212,15 +207,7 @@ 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, diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go new file mode 100644 index 0000000000..388a0640cb --- /dev/null +++ b/pkg/inventory/features.go @@ -0,0 +1,189 @@ +package inventory + +import ( + "context" + "fmt" + "os" + "slices" + "sync" +) + +// FeatureFlag identifies a feature consistently across inventory consumers. +type FeatureFlag string + +// FeatureFlagChecker resolves one feature flag for the current request. +type FeatureFlagChecker func(ctx context.Context, flag FeatureFlag) (bool, error) + +// FeatureResolver returns the resolved value of a feature flag. +// Implementations absorb resolution errors and fail closed. +type FeatureResolver func(flag FeatureFlag) bool + +// FeaturePredicate determines whether an inventory item is available. +type FeaturePredicate func(featureAsBool FeatureResolver) bool + +// FeatureRule declares the feature flags used by an availability predicate. +// The declaration lets the service resolve and deduplicate checks before the +// predicate runs, while the predicate retains normal Go boolean semantics. +type FeatureRule struct { + features []FeatureFlag + featureSet map[FeatureFlag]struct{} + predicate FeaturePredicate +} + +// NewFeatureRule creates an availability rule over the supplied feature flags. +func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureRule { + declared := make([]FeatureFlag, 0, len(features)) + featureSet := make(map[FeatureFlag]struct{}, len(features)) + for _, feature := range features { + if feature == "" { + continue + } + if _, ok := featureSet[feature]; ok { + continue + } + featureSet[feature] = struct{}{} + declared = append(declared, feature) + } + return FeatureRule{ + features: declared, + featureSet: featureSet, + predicate: predicate, + } +} + +// Features returns the feature flags referenced by the rule. +func (r FeatureRule) Features() []FeatureFlag { + return append([]FeatureFlag(nil), r.features...) +} + +// IsZero reports whether no feature availability rule is configured. +func (r FeatureRule) IsZero() bool { + return r.predicate == nil +} + +// Enabled evaluates the rule against resolved feature values. +func (r FeatureRule) Enabled(featureAsBool FeatureResolver) bool { + if r.predicate == nil { + return true + } + if featureAsBool == nil { + return false + } + + var undeclared FeatureFlag + enabled := r.predicate(func(feature FeatureFlag) bool { + if _, ok := r.featureSet[feature]; !ok { + undeclared = feature + return false + } + return featureAsBool(feature) + }) + if undeclared != "" { + fmt.Fprintf(os.Stderr, "Feature rule used undeclared feature %q\n", undeclared) + return false + } + return enabled +} + +type featureStateContextKey struct{} + +type featureState struct { + checker FeatureFlagChecker + + mu sync.Mutex + values map[FeatureFlag]bool +} + +func newFeatureState(checker FeatureFlagChecker) *featureState { + return &featureState{ + checker: checker, + values: make(map[FeatureFlag]bool), + } +} + +func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool { + if feature == "" || s.checker == nil { + return false + } + + s.mu.Lock() + defer s.mu.Unlock() + + if enabled, ok := s.values[feature]; ok { + return enabled + } + + enabled, err := s.checker(ctx, feature) + if err != nil { + fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", feature, err) + enabled = false + } + s.values[feature] = enabled + return enabled +} + +// WithResolvedFeatures resolves the deduplicated feature names into state owned +// by the returned context. Repeated calls extend and reuse that state. +func WithResolvedFeatures(ctx context.Context, checker FeatureFlagChecker, features []FeatureFlag) context.Context { + state, _ := ctx.Value(featureStateContextKey{}).(*featureState) + if state == nil { + if checker == nil { + return ctx + } + state = newFeatureState(checker) + ctx = context.WithValue(ctx, featureStateContextKey{}, state) + } + + features = append([]FeatureFlag(nil), features...) + slices.Sort(features) + for _, feature := range features { + state.enabled(ctx, feature) + } + return ctx +} + +// ResolveFeature returns a feature value from request-owned resolution state. +// Features not resolved up front are resolved lazily and cached. +func ResolveFeature(ctx context.Context, checker FeatureFlagChecker, feature FeatureFlag) bool { + if feature == "" { + return false + } + if state, _ := ctx.Value(featureStateContextKey{}).(*featureState); state != nil { + return state.enabled(ctx, feature) + } + if checker == nil { + return false + } + return newFeatureState(checker).enabled(ctx, feature) +} + +func featureResolver(ctx context.Context, checker FeatureFlagChecker) FeatureResolver { + if state, _ := ctx.Value(featureStateContextKey{}).(*featureState); state != nil { + return func(feature FeatureFlag) bool { + return state.enabled(ctx, feature) + } + } + if checker == nil { + return func(FeatureFlag) bool { return false } + } + state := newFeatureState(checker) + return func(feature FeatureFlag) bool { + return state.enabled(ctx, feature) + } +} + +func collectFeatures(rules ...FeatureRule) []FeatureFlag { + seen := make(map[FeatureFlag]struct{}) + for _, rule := range rules { + for _, feature := range rule.features { + seen[feature] = struct{}{} + } + } + + features := make([]FeatureFlag, 0, len(seen)) + for feature := range seen { + features = append(features, feature) + } + slices.Sort(features) + return features +} diff --git a/pkg/inventory/features_test.go b/pkg/inventory/features_test.go new file mode 100644 index 0000000000..6d7bba30d9 --- /dev/null +++ b/pkg/inventory/features_test.go @@ -0,0 +1,95 @@ +package inventory + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFeatureRuleSupportsBooleanExpressions(t *testing.T) { + rule := NewFeatureRule( + []FeatureFlag{"x", "y"}, + func(featureAsBool FeatureResolver) bool { + return !featureAsBool("x") || !featureAsBool("y") + }, + ) + + tests := []struct { + name string + values map[FeatureFlag]bool + want bool + }{ + {name: "neither enabled", values: map[FeatureFlag]bool{}, want: true}, + {name: "one enabled", values: map[FeatureFlag]bool{"x": true}, want: true}, + {name: "both enabled", values: map[FeatureFlag]bool{"x": true, "y": true}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, rule.Enabled(func(flag FeatureFlag) bool { + return tt.values[flag] + })) + }) + } + +} + +func TestFeatureRuleFailsClosedForUndeclaredFeature(t *testing.T) { + rule := NewFeatureRule( + []FeatureFlag{"declared"}, + func(featureAsBool FeatureResolver) bool { + return featureAsBool("undeclared") + }, + ) + + assert.False(t, rule.Enabled(func(FeatureFlag) bool { return true })) +} + +func TestResolvedFeaturesDeduplicateAndCacheChecks(t *testing.T) { + calls := make(map[FeatureFlag]int) + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + calls[flag]++ + if flag == "error" { + return false, errors.New("failed") + } + return flag == "enabled", nil + } + + ctx := WithResolvedFeatures( + context.Background(), + checker, + []FeatureFlag{"enabled", "disabled", "enabled", "error"}, + ) + + assert.True(t, ResolveFeature(ctx, nil, "enabled")) + assert.False(t, ResolveFeature(ctx, checker, "disabled")) + assert.False(t, ResolveFeature(ctx, checker, "error")) + assert.False(t, ResolveFeature(ctx, checker, "lazy")) + assert.False(t, ResolveFeature(ctx, checker, "lazy")) + + require.Equal(t, map[FeatureFlag]int{ + "enabled": 1, + "disabled": 1, + "error": 1, + "lazy": 1, + }, calls) +} + +func TestResolvedFeaturesAllowNilChecker(t *testing.T) { + ctx := WithResolvedFeatures(context.Background(), nil, []FeatureFlag{"feature"}) + assert.False(t, ResolveFeature(ctx, nil, "feature")) +} + +func TestLazyFeatureResolutionUsesLiveContext(t *testing.T) { + type contextKey struct{} + checker := func(ctx context.Context, _ FeatureFlag) (bool, error) { + enabled, _ := ctx.Value(contextKey{}).(bool) + return enabled, nil + } + + ctx := WithResolvedFeatures(context.Background(), checker, nil) + ctx = context.WithValue(ctx, contextKey{}, true) + assert.True(t, ResolveFeature(ctx, checker, "handler_only")) +} diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 96e46f215f..bc8649dd87 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -4,15 +4,9 @@ import ( "context" "fmt" "os" - "slices" "sort" ) -// FeatureFlagChecker is a function that checks if a feature flag is enabled. -// The context can be used to extract actor/user information for flag evaluation. -// Returns (enabled, error). If error occurs, the caller should log and treat as false. -type FeatureFlagChecker func(ctx context.Context, flagName string) (bool, error) - // isToolsetEnabled checks if a toolset is enabled based on current filters. func (r *Inventory) isToolsetEnabled(toolsetID ToolsetID) bool { // Check enabled toolsets filter @@ -24,74 +18,18 @@ func (r *Inventory) isToolsetEnabled(toolsetID ToolsetID) bool { // checkFeatureFlag checks a feature flag using the feature checker. // Returns false if checker is nil or returns an error (errors are logged). -func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName string) bool { - if r.featureChecker == nil || flagName == "" { - return false - } - enabled, err := r.featureChecker(ctx, flagName) - if err != nil { - fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err) - return false - } - return enabled -} - -// 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. -// - Every FeatureFlagEnableAll entry must also be enabled. -// - If FeatureFlagDisable is non-empty, the item is excluded if any listed flag is enabled. -func featureFlagAllowed(ctx context.Context, checker FeatureFlagChecker, enableFlag string, disableFlags []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 - } - return !slices.ContainsFunc(disableFlags, check) -} - -// createFeatureFlagFilter returns a ToolFilter that gates tools on their -// FeatureFlagEnable / FeatureFlagEnableAll / 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) { - if !featureFlagAllowed(ctx, checker, tool.FeatureFlagEnable, tool.FeatureFlagDisable) { - return false, nil - } - for _, flag := range tool.FeatureFlagEnableAll { - if !featureFlagAllowed(ctx, checker, flag, nil) { - return false, nil - } - } - return true, nil - } +func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName FeatureFlag) bool { + return ResolveFeature(ctx, r.featureChecker, flagName) } // isToolEnabled checks if a specific tool is enabled based on current filters. // Filter evaluation order: // 1. Tool.Enabled (tool self-filtering) -// 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 { +// 2. Functional feature rule +// 3. Read-only filter +// 4. Builder filters (via WithFilter) +// 5. Toolset/additional tools +func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, featureAsBool FeatureResolver) bool { // 1. Check tool's own Enabled function first if tool.Enabled != nil { enabled, err := tool.Enabled(ctx) @@ -103,11 +41,15 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { return false } } - // 2. Check read-only filter (applies to all tools) + // 2. Check feature availability. + if r.featureChecker != nil && !tool.FeatureRule.Enabled(featureAsBool) { + return false + } + // 3. Check read-only filter (applies to all tools) if r.readOnly && !tool.IsReadOnly() { return false } - // 3. Apply builder filters (includes the feature-flag filter when set) + // 4. Apply builder filters. for _, filter := range r.filters { allowed, err := filter(ctx, tool) if err != nil { @@ -118,11 +60,11 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool) bool { return false } } - // 4. Check if tool is in additionalTools (bypasses toolset filter) + // 5. Check if tool is in additionalTools (bypasses toolset filter) if r.additionalTools != nil && r.additionalTools[tool.Tool.Name] { return true } - // 4. Check toolset filter + // 6. Check toolset filter if !r.isToolsetEnabled(tool.Toolset.ID) { return false } @@ -154,10 +96,12 @@ func sortTools(tools []ServerTool) { // sorted deterministically by toolset ID, then tool name. // The context is used for feature flag evaluation. func (r *Inventory) AvailableTools(ctx context.Context) []ServerTool { + ctx = WithResolvedFeatures(ctx, r.featureChecker, r.requiredToolFeatures()) + featureAsBool := featureResolver(ctx, r.featureChecker) var result []ServerTool for i := range r.tools { tool := &r.tools[i] - if r.isToolEnabled(ctx, tool) { + if r.isToolEnabled(ctx, tool, featureAsBool) { result = append(result, *tool) } } @@ -179,14 +123,12 @@ func sortResourceTemplates(resourceTemplates []ServerResourceTemplate) { // sorted deterministically by toolset ID, then template name. // The context is used for feature flag evaluation. func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerResourceTemplate { + ctx = WithResolvedFeatures(ctx, r.featureChecker, r.requiredResourceFeatures()) + featureAsBool := featureResolver(ctx, r.featureChecker) var result []ServerResourceTemplate for i := range r.resourceTemplates { res := &r.resourceTemplates[i] - // 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) { + if r.featureChecker != nil && !res.FeatureRule.Enabled(featureAsBool) { continue } if r.isToolsetEnabled(res.Toolset.ID) { @@ -211,12 +153,12 @@ func sortPrompts(prompts []ServerPrompt) { // sorted deterministically by toolset ID, then prompt name. // The context is used for feature flag evaluation. func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { + ctx = WithResolvedFeatures(ctx, r.featureChecker, r.requiredPromptFeatures()) + featureAsBool := featureResolver(ctx, r.featureChecker) var result []ServerPrompt for i := range r.prompts { prompt := &r.prompts[i] - // 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) { + if r.featureChecker != nil && !prompt.FeatureRule.Enabled(featureAsBool) { continue } if r.isToolsetEnabled(prompt.Toolset.ID) { diff --git a/pkg/inventory/prompts.go b/pkg/inventory/prompts.go index d929578e83..768a76f457 100644 --- a/pkg/inventory/prompts.go +++ b/pkg/inventory/prompts.go @@ -8,12 +8,8 @@ type ServerPrompt struct { Handler mcp.PromptHandler // Toolset identifies which toolset this prompt belongs to Toolset ToolsetMetadata - // FeatureFlagEnable specifies a feature flag that must be enabled for this prompt - // to be available. If set and the flag is not enabled, the prompt is omitted. - FeatureFlagEnable string - // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this - // prompt to be omitted. Used to disable prompts when a feature flag is on. - FeatureFlagDisable []string + // FeatureRule controls whether this prompt is available. + FeatureRule FeatureRule } // NewServerPrompt creates a new ServerPrompt with toolset metadata. diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 3483d448cb..5e658b5c9b 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -188,13 +188,71 @@ func (r *Inventory) ToolsetDescriptions() map[ToolsetID]string { // capability is unknown (e.g. stdio paths that do not populate the // context flag) the feature-flag gate is the sole source of truth. func (r *Inventory) ToolsForRegistration(ctx context.Context) []ServerTool { + ctx = WithResolvedFeatures(ctx, r.featureChecker, r.RequiredFeatures()) tools := r.AvailableTools(ctx) - if shouldStripMCPAppsMetadata(ctx, r.checkFeatureFlag(ctx, mcpAppsFeatureFlag)) { + if r.usesMCPAppsMetadata() && shouldStripMCPAppsMetadata(ctx, r.checkFeatureFlag(ctx, mcpAppsFeatureFlag)) { tools = stripMCPAppsMetadata(tools) } return tools } +func (r *Inventory) requiredToolFeatures() []FeatureFlag { + rules := make([]FeatureRule, 0, len(r.tools)) + for i := range r.tools { + rules = append(rules, r.tools[i].FeatureRule) + } + return collectFeatures(rules...) +} + +func (r *Inventory) requiredResourceFeatures() []FeatureFlag { + rules := make([]FeatureRule, 0, len(r.resourceTemplates)) + for i := range r.resourceTemplates { + rules = append(rules, r.resourceTemplates[i].FeatureRule) + } + return collectFeatures(rules...) +} + +func (r *Inventory) requiredPromptFeatures() []FeatureFlag { + rules := make([]FeatureRule, 0, len(r.prompts)) + for i := range r.prompts { + rules = append(rules, r.prompts[i].FeatureRule) + } + return collectFeatures(rules...) +} + +// RequiredFeatures returns the deduplicated feature flags used to expose the +// inventory's current tools, resources, and prompts. +func (r *Inventory) RequiredFeatures() []FeatureFlag { + features := append(r.requiredToolFeatures(), r.requiredResourceFeatures()...) + features = append(features, r.requiredPromptFeatures()...) + if r.usesMCPAppsMetadata() { + features = append(features, mcpAppsFeatureFlag) + } + + seen := make(map[FeatureFlag]struct{}, len(features)) + result := make([]FeatureFlag, 0, len(features)) + for _, feature := range features { + if _, ok := seen[feature]; ok { + continue + } + seen[feature] = struct{}{} + result = append(result, feature) + } + slices.Sort(result) + return result +} + +func (r *Inventory) usesMCPAppsMetadata() bool { + for i := range r.tools { + for _, key := range mcpAppsMetaKeys { + if _, ok := r.tools[i].Tool.Meta[key]; ok { + return true + } + } + } + return false +} + // shouldStripMCPAppsMetadata centralises the strip decision so the same logic // is exercised by tests and by RegisterTools. func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bool { diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 0a966b289b..e3b12df123 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1044,13 +1044,20 @@ func TestMCPMethodConstants(t *testing.T) { } } -// mockToolWithFlags creates a ServerTool with feature flags for testing +// mockToolWithFlags creates a ServerTool with a functional feature rule for testing. func mockToolWithFlags(name string, toolsetID string, readOnly bool, enableFlag, disableFlag string) ServerTool { tool := mockTool(name, toolsetID, readOnly) - tool.FeatureFlagEnable = enableFlag + features := make([]FeatureFlag, 0, 2) + if enableFlag != "" { + features = append(features, FeatureFlag(enableFlag)) + } if disableFlag != "" { - tool.FeatureFlagDisable = []string{disableFlag} + features = append(features, FeatureFlag(disableFlag)) } + tool.FeatureRule = NewFeatureRule(features, func(featureAsBool FeatureResolver) bool { + return (enableFlag == "" || featureAsBool(FeatureFlag(enableFlag))) && + (disableFlag == "" || !featureAsBool(FeatureFlag(disableFlag))) + }) return tool } @@ -1067,8 +1074,8 @@ func TestFeatureFlagEnable(t *testing.T) { t.Fatalf("Expected 2 tools without feature checker (filtering skipped), got %d", len(available)) } - // With feature checker returning false, FeatureFlagEnable tool is excluded - checkerFalse := func(_ context.Context, _ string) (bool, error) { return false, nil } + // With feature checker returning false, the feature-gated tool is excluded. + checkerFalse := func(_ context.Context, _ FeatureFlag) (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 { @@ -1079,7 +1086,7 @@ func TestFeatureFlagEnable(t *testing.T) { } // With feature checker returning true for "my_feature", tool should be included - checkerTrue := func(_ context.Context, flag string) (bool, error) { + checkerTrue := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "my_feature", nil } regTrue := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerTrue)) @@ -1095,7 +1102,7 @@ func TestFeatureFlagDisable(t *testing.T) { mockToolWithFlags("disabled_by_flag", "toolset1", true, "", "kill_switch"), } - // Without feature checker, tool with FeatureFlagDisable should be included (flag is false) + // Without feature checker, feature filtering is skipped. reg := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"})) available := reg.AvailableTools(context.Background()) if len(available) != 2 { @@ -1103,7 +1110,7 @@ func TestFeatureFlagDisable(t *testing.T) { } // With feature checker returning true for "kill_switch", tool should be excluded - checkerTrue := func(_ context.Context, flag string) (bool, error) { + checkerTrue := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "kill_switch", nil } regFiltered := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checkerTrue)) @@ -1123,21 +1130,21 @@ func TestFeatureFlagBoth(t *testing.T) { } // Enable flag not set -> excluded - checker1 := func(_ context.Context, _ string) (bool, error) { return false, nil } + checker1 := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } reg1 := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checker1)) if len(reg1.AvailableTools(context.Background())) != 0 { t.Error("Tool should be excluded when enable flag is false") } // Enable flag set, disable flag not set -> included - checker2 := func(_ context.Context, flag string) (bool, error) { return flag == "new_feature", nil } + checker2 := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "new_feature", nil } reg2 := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checker2)) if len(reg2.AvailableTools(context.Background())) != 1 { t.Error("Tool should be included when enable flag is true and disable flag is false") } // Enable flag set, disable flag also set -> excluded (disable wins) - checker3 := func(_ context.Context, _ string) (bool, error) { return true, nil } + checker3 := func(_ context.Context, _ FeatureFlag) (bool, error) { return true, nil } reg3 := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"}).WithFeatureChecker(checker3)) if len(reg3.AvailableTools(context.Background())) != 0 { t.Error("Tool should be excluded when both flags are true (disable wins)") @@ -1150,7 +1157,7 @@ func TestFeatureFlagError(t *testing.T) { } // Checker that returns error should treat as false (tool excluded) - checkerError := func(_ context.Context, _ string) (bool, error) { + checkerError := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, fmt.Errorf("simulated error") } reg := mustBuild(t, NewBuilder().SetTools(tools).WithFeatureChecker(checkerError)) @@ -1164,9 +1171,11 @@ func TestFeatureFlagResources(t *testing.T) { resources := []ServerResourceTemplate{ mockResource("always_available", "toolset1", "uri1"), { - Template: mcp.ResourceTemplate{Name: "needs_flag", URITemplate: "uri2"}, - Toolset: testToolsetMetadata("toolset1"), - FeatureFlagEnable: "my_feature", + Template: mcp.ResourceTemplate{Name: "needs_flag", URITemplate: "uri2"}, + Toolset: testToolsetMetadata("toolset1"), + FeatureRule: NewFeatureRule([]FeatureFlag{"my_feature"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("my_feature") + }), }, } @@ -1178,7 +1187,7 @@ func TestFeatureFlagResources(t *testing.T) { } // With checker returning true, both should be included - checker := func(_ context.Context, _ string) (bool, error) { return true, nil } + checker := func(_ context.Context, _ FeatureFlag) (bool, error) { return true, nil } regWithChecker := mustBuild(t, NewBuilder().SetResources(resources).WithToolsets([]string{"all"}).WithFeatureChecker(checker)) if len(regWithChecker.AvailableResourceTemplates(context.Background())) != 2 { t.Errorf("Expected 2 resources with checker, got %d", len(regWithChecker.AvailableResourceTemplates(context.Background()))) @@ -1189,9 +1198,11 @@ func TestFeatureFlagPrompts(t *testing.T) { prompts := []ServerPrompt{ mockPrompt("always_available", "toolset1"), { - Prompt: mcp.Prompt{Name: "needs_flag"}, - Toolset: testToolsetMetadata("toolset1"), - FeatureFlagEnable: "my_feature", + Prompt: mcp.Prompt{Name: "needs_flag"}, + Toolset: testToolsetMetadata("toolset1"), + FeatureRule: NewFeatureRule([]FeatureFlag{"my_feature"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("my_feature") + }), }, } @@ -1203,7 +1214,7 @@ func TestFeatureFlagPrompts(t *testing.T) { } // With checker returning true, both should be included - checker := func(_ context.Context, _ string) (bool, error) { return true, nil } + checker := func(_ context.Context, _ FeatureFlag) (bool, error) { return true, nil } regWithChecker := mustBuild(t, NewBuilder().SetPrompts(prompts).WithToolsets([]string{"all"}).WithFeatureChecker(checker)) if len(regWithChecker.AvailablePrompts(context.Background())) != 2 { t.Errorf("Expected 2 prompts with checker, got %d", len(regWithChecker.AvailablePrompts(context.Background()))) @@ -1485,7 +1496,7 @@ 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 } + checkerOff := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } reg1 := mustBuild(t, NewBuilder(). SetTools([]ServerTool{tool}). WithToolsets([]string{"all"}). @@ -1496,7 +1507,7 @@ func TestEnabledAndFeatureFlagInteraction(t *testing.T) { } // Feature flag enabled - tool should be included - checker := func(_ context.Context, flag string) (bool, error) { + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "my_feature", nil } reg2 := mustBuild(t, NewBuilder(). @@ -1555,7 +1566,7 @@ func TestAllFiltersInteraction(t *testing.T) { return true, nil } - checker := func(_ context.Context, flag string) (bool, error) { + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "my_feature", nil } @@ -1655,10 +1666,10 @@ func TestFilteredToolsMatchesAvailableTools(t *testing.T) { func TestFilteringOrder(t *testing.T) { // Test that filters are applied in the correct order: // 1. Tool.Enabled - // 2. Read-only - // 3. Builder filters (feature-flag filter is at the head of this list - // when WithFeatureChecker is set) - // 4. Toolset/additional tools + // 2. Feature rule + // 3. Read-only + // 4. Builder filters + // 5. Toolset/additional tools callOrder := []string{} @@ -1673,7 +1684,7 @@ func TestFilteringOrder(t *testing.T) { return true, nil } - checker := func(_ context.Context, _ string) (bool, error) { + checker := func(_ context.Context, _ FeatureFlag) (bool, error) { callOrder = append(callOrder, "FeatureFlag") return true, nil } @@ -1685,15 +1696,13 @@ func TestFilteringOrder(t *testing.T) { WithFeatureChecker(checker). WithFilter(filter)) - // Reset call order — Build() may call the checker for MCP Apps metadata. // We're testing the AvailableTools filter order here. callOrder = callOrder[:0] _ = reg.AvailableTools(context.Background()) - // 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"} + // Declared features resolve first, then Enabled runs before read-only stops. + expectedOrder := []string{"FeatureFlag", "Enabled"} if len(callOrder) != len(expectedOrder) { t.Errorf("Expected %d checks, got %d: %v", len(expectedOrder), len(callOrder), callOrder) } @@ -1707,8 +1716,8 @@ func TestFilteringOrder(t *testing.T) { func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { // Simulate the get_job_logs scenario: two tools with the same name but different feature flags - // - "get_job_logs" with FeatureFlagDisable (available when flag is OFF) - // - "get_job_logs" with FeatureFlagEnable (available when flag is ON) + // - one "get_job_logs" variant available when the flag is off + // - one "get_job_logs" variant available when the flag is on tools := []ServerTool{ mockToolWithFlags("get_job_logs", "actions", true, "", "consolidated_flag"), // disabled when flag is ON mockToolWithFlags("get_job_logs", "actions", true, "consolidated_flag", ""), // enabled when flag is ON @@ -1716,7 +1725,7 @@ 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 } + checkerOff := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). WithToolsets([]string{"all"}). @@ -1726,13 +1735,12 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { if len(availableOff) != 1 { t.Fatalf("Flag OFF: Expected 1 tool, got %d", len(availableOff)) } - if len(availableOff[0].FeatureFlagDisable) != 1 || availableOff[0].FeatureFlagDisable[0] != "consolidated_flag" { - t.Errorf("Flag OFF: Expected tool with FeatureFlagDisable, got FeatureFlagEnable=%q, FeatureFlagDisable=%v", - availableOff[0].FeatureFlagEnable, availableOff[0].FeatureFlagDisable) + if !availableOff[0].FeatureRule.Enabled(func(FeatureFlag) bool { return false }) { + t.Error("Flag OFF: expected the flag-off feature rule") } // Test 2: Flag is ON - second tool variant should be available - checker := func(_ context.Context, flag string) (bool, error) { + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "consolidated_flag", nil } regFlagOn := mustBuild(t, NewBuilder(). @@ -1744,16 +1752,15 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { if len(availableOn) != 1 { t.Fatalf("Flag ON: Expected 1 tool, got %d", len(availableOn)) } - if availableOn[0].FeatureFlagEnable != "consolidated_flag" { - t.Errorf("Flag ON: Expected tool with FeatureFlagEnable, got FeatureFlagEnable=%q, FeatureFlagDisable=%v", - availableOn[0].FeatureFlagEnable, availableOn[0].FeatureFlagDisable) + if !availableOn[0].FeatureRule.Enabled(func(FeatureFlag) bool { return true }) { + t.Error("Flag ON: expected the flag-on feature rule") } } // TestWithTools_DeprecatedAliasAndFeatureFlag tests that deprecated aliases work correctly // when the old tool is controlled by a feature flag. This covers the scenario where: -// - Old tool "old_tool" has FeatureFlagDisable="my_flag" (available when flag is OFF) -// - New tool "new_tool" has FeatureFlagEnable="my_flag" (available when flag is ON) +// - Old tool "old_tool" is available when the flag is off +// - New tool "new_tool" is available when the flag is on // - Deprecated alias maps "old_tool" -> "new_tool" // - User specifies --tools=old_tool // Expected behavior: @@ -1770,7 +1777,7 @@ 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 } + checkerOff := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). WithDeprecatedAliases(deprecatedAliases). @@ -1786,7 +1793,7 @@ func TestWithTools_DeprecatedAliasAndFeatureFlag(t *testing.T) { } // Test 2: Flag ON - new_tool should be available via alias resolution - checker := func(_ context.Context, flag string) (bool, error) { + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "my_flag", nil } regFlagOn := mustBuild(t, NewBuilder(). @@ -1849,7 +1856,7 @@ func TestWithMCPApps_EnabledPreservesUIMetadata(t *testing.T) { }) // Feature checker enables MCP Apps - UI meta should be preserved - mcpAppsChecker := func(_ context.Context, flag string) (bool, error) { + mcpAppsChecker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == mcpAppsFeatureFlag, nil } reg := mustBuild(t, NewBuilder(). diff --git a/pkg/inventory/resources.go b/pkg/inventory/resources.go index 2dd07ae0fe..62a846120f 100644 --- a/pkg/inventory/resources.go +++ b/pkg/inventory/resources.go @@ -16,12 +16,8 @@ type ServerResourceTemplate struct { HandlerFunc ResourceHandlerFunc // Toolset identifies which toolset this resource belongs to Toolset ToolsetMetadata - // FeatureFlagEnable specifies a feature flag that must be enabled for this resource - // to be available. If set and the flag is not enabled, the resource is omitted. - FeatureFlagEnable string - // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this - // resource to be omitted. Used to disable resources when a feature flag is on. - FeatureFlagDisable []string + // FeatureRule controls whether this resource is available. + FeatureRule FeatureRule } // HasHandler returns true if this resource has a handler function. diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 9c458c3d12..2bc2769593 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -94,17 +94,9 @@ type ServerTool struct { // and handlers are only created when needed. HandlerFunc HandlerFunc - // FeatureFlagEnable specifies a feature flag that must be enabled for this tool - // to be available. If set and the flag is not enabled, the tool is omitted. - FeatureFlagEnable string - - // FeatureFlagEnableAll specifies additional feature flags that must all be enabled - // for this tool to be available. - FeatureFlagEnableAll []string - - // FeatureFlagDisable specifies feature flags that, when any is enabled, cause this - // tool to be omitted. Used to disable tools when a feature flag is on. - FeatureFlagDisable []string + // FeatureRule declares and evaluates the feature flags that control whether + // this tool is available. Its zero value leaves the tool available. + FeatureRule FeatureRule // Enabled is an optional function called at build/filter time to determine // if this tool should be available. If nil, the tool is considered enabled diff --git a/script/print-mcp-diff-configs/main.go b/script/print-mcp-diff-configs/main.go index 421c9fce41..ada18c4063 100644 --- a/script/print-mcp-diff-configs/main.go +++ b/script/print-mcp-diff-configs/main.go @@ -141,7 +141,7 @@ func baseEntries() []baseEntry { }}, } - flags := append([]string(nil), github.AllowedFeatureFlags...) + flags := github.HeaderAllowedFeatureFlags() sort.Strings(flags) for _, f := range flags { entries = append(entries, baseEntry{ @@ -208,7 +208,7 @@ func (s settings) toHeaders() map[string]string { } func firstFeatureFlag() string { - flags := append([]string(nil), github.AllowedFeatureFlags...) + flags := github.HeaderAllowedFeatureFlags() if len(flags) == 0 { return "" } From 7bab7db1bc9ef8350b74b4239934318074f961be Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 27 Aug 2026 12:06:46 +0200 Subject: [PATCH 2/2] fix(features): address resolver review feedback Keep legacy string APIs compatible, seed feature state from each inventory's checker, persist caching for stdio calls, and fail closed for empty undeclared flags. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- cmd/github-mcp-server/feature_flag_docs.go | 10 ++--- internal/ghmcp/server.go | 4 +- pkg/github/feature_flags.go | 47 +++++++++---------- pkg/github/feature_flags_test.go | 52 +++++++++++----------- pkg/github/granular_tools_test.go | 11 ++--- pkg/github/issues_test.go | 2 +- pkg/github/pullrequests.go | 8 ++-- pkg/github/pullrequests_granular.go | 8 ++-- pkg/github/server.go | 9 ++++ pkg/github/server_test.go | 21 +++++++++ pkg/github/tools.go | 10 ++--- pkg/github/tools_validation_test.go | 2 +- pkg/http/handler.go | 4 +- pkg/http/server.go | 4 +- pkg/http/server_test.go | 44 ++++++++---------- pkg/inventory/features.go | 4 +- pkg/inventory/features_test.go | 11 +++++ pkg/inventory/registry.go | 12 +++++ 18 files changed, 149 insertions(+), 114 deletions(-) diff --git a/cmd/github-mcp-server/feature_flag_docs.go b/cmd/github-mcp-server/feature_flag_docs.go index a0ecea9d2a..7c17e58222 100644 --- a/cmd/github-mcp-server/feature_flag_docs.go +++ b/cmd/github-mcp-server/feature_flag_docs.go @@ -33,7 +33,7 @@ func generateFeatureFlagsDocs(docsPath string) error { // whose registration or definition differs from the default user experience. // Each affected tool is printed with its full schema using the same writer // used by the README so the output style stays consistent. -func generateFlaggedToolsDoc(flags []inventory.FeatureFlag, emptyMessage string) string { +func generateFlaggedToolsDoc(flags []string, emptyMessage string) string { t, _ := translations.TranslationHelper() defaultTools := indexToolsByName(buildInventoryWithFlags(t, nil).ToolsForRegistration(context.Background())) @@ -73,8 +73,8 @@ func generateFlaggedToolsDoc(flags []inventory.FeatureFlag, emptyMessage string) // differs from the default-flagged inventory when only the given flag is on, // plus tools that exist only in the flag-on inventory. Results are sorted by // tool name. -func flaggedToolDiff(t translations.TranslationHelperFunc, flag inventory.FeatureFlag, defaultTools map[string]inventory.ServerTool) []inventory.ServerTool { - flagTools := buildInventoryWithFlags(t, map[inventory.FeatureFlag]bool{flag: true}).ToolsForRegistration(context.Background()) +func flaggedToolDiff(t translations.TranslationHelperFunc, flag string, defaultTools map[string]inventory.ServerTool) []inventory.ServerTool { + flagTools := buildInventoryWithFlags(t, map[string]bool{flag: true}).ToolsForRegistration(context.Background()) out := make([]inventory.ServerTool, 0) seen := make(map[string]struct{}, len(flagTools)) @@ -99,9 +99,9 @@ func flaggedToolDiff(t translations.TranslationHelperFunc, flag inventory.Featur // buildInventoryWithFlags constructs an inventory whose feature checker treats // the given flags as enabled and every other flag as disabled. Passing nil // produces the default-flagged inventory. -func buildInventoryWithFlags(t translations.TranslationHelperFunc, enabled map[inventory.FeatureFlag]bool) *inventory.Inventory { +func buildInventoryWithFlags(t translations.TranslationHelperFunc, enabled map[string]bool) *inventory.Inventory { checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) { - return enabled[flag], nil + return enabled[string(flag)], nil } inv, _ := github.NewInventory(t). WithToolsets([]string{"all"}). diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 02a6fb6038..91de092630 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -435,9 +435,9 @@ func RunStdioServer(cfg StdioServerConfig) error { // using the centralized ResolveFeatureFlags function. For the local server, // features are resolved once at startup from --features CLI flag and insiders mode. func createFeatureChecker(enabledFeatures []string, insidersMode bool) inventory.FeatureFlagChecker { - featureSet := github.ResolveFeatureFlags(github.FeatureFlagsFromStrings(enabledFeatures), insidersMode) + featureSet := github.ResolveFeatureFlags(enabledFeatures, insidersMode) return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { - return featureSet[flagName], nil + return featureSet[string(flagName)], nil } } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index e8219075f8..858bd075cb 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -7,45 +7,45 @@ import ( ) // MCPAppsFeatureFlag is the feature flag name for MCP Apps (interactive UI forms). -const MCPAppsFeatureFlag inventory.FeatureFlag = "remote_mcp_ui_apps" +const MCPAppsFeatureFlag = "remote_mcp_ui_apps" // MCPAppsDisableFormDeferralFeatureFlag disables handing write-tool calls off // to MCP App forms while preserving MCP Apps UI metadata and result views. -const MCPAppsDisableFormDeferralFeatureFlag inventory.FeatureFlag = "mcp_apps_disable_form_deferral" +const MCPAppsDisableFormDeferralFeatureFlag = "mcp_apps_disable_form_deferral" // FeatureFlagCSVOutput is the feature flag name for CSV output on list tools. -const FeatureFlagCSVOutput inventory.FeatureFlag = "csv_output" +const FeatureFlagCSVOutput = "csv_output" // FeatureFlagIFCLabels is the feature flag name for IFC security labels in tool results. -const FeatureFlagIFCLabels inventory.FeatureFlag = "ifc_labels" +const FeatureFlagIFCLabels = "ifc_labels" // FeatureFlagFileBlame is the feature flag name for the get_file_blame tool, // which exposes git blame information for a file. It is gated so the extra tool // is not advertised by default, keeping the tool surface small unless opted in. -const FeatureFlagFileBlame inventory.FeatureFlag = "file_blame" +const FeatureFlagFileBlame = "file_blame" // FeatureFlagIssueDependencies is the feature flag name for the issue dependency // tools (issue_dependency_read / issue_dependency_write), which read and edit an // issue's blocked-by / blocking relationships. It is gated so these tools are not // advertised in the default surface, keeping the fixed tool-schema cost small // unless explicitly opted in. -const FeatureFlagIssueDependencies inventory.FeatureFlag = "issue_dependencies" +const FeatureFlagIssueDependencies = "issue_dependencies" // FeatureFlagDuplicateDetection is the feature flag name for the find_duplicate // tool, which returns ranked duplicate candidates for an existing issue. It is // gated so the extra tool is not advertised by default, and is deliberately // excluded from insiders mode so duplicate detection is only ever an explicit // opt-in. -const FeatureFlagDuplicateDetection inventory.FeatureFlag = "duplicate_detection" +const FeatureFlagDuplicateDetection = "duplicate_detection" // FeatureFlagThreadResolutionReason exposes resolution reasons for Copilot review threads. -const FeatureFlagThreadResolutionReason inventory.FeatureFlag = "thread_resolution_reason" +const FeatureFlagThreadResolutionReason = "thread_resolution_reason" // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. // This is the single source of truth for which flags are user-controllable. -var AllowedFeatureFlags = []inventory.FeatureFlag{ +var AllowedFeatureFlags = []string{ MCPAppsFeatureFlag, MCPAppsDisableFormDeferralFeatureFlag, FeatureFlagCSVOutput, @@ -62,7 +62,7 @@ var AllowedFeatureFlags = []inventory.FeatureFlag{ // When insiders mode is active, all flags in this list are treated as enabled. // This is the single source of truth for what "insiders" means in terms of // feature flag expansion. -var InsidersFeatureFlags = []inventory.FeatureFlag{ +var InsidersFeatureFlags = []string{ MCPAppsFeatureFlag, FeatureFlagCSVOutput, FeatureFlagFileBlame, @@ -74,20 +74,22 @@ type FeatureFlags struct { LockdownMode bool } -func featureEnabledRule(feature inventory.FeatureFlag) inventory.FeatureRule { +func featureEnabledRule(feature string) inventory.FeatureRule { + flag := inventory.FeatureFlag(feature) return inventory.NewFeatureRule( - []inventory.FeatureFlag{feature}, + []inventory.FeatureFlag{flag}, func(featureAsBool inventory.FeatureResolver) bool { - return featureAsBool(feature) + return featureAsBool(flag) }, ) } -func featureDisabledRule(feature inventory.FeatureFlag) inventory.FeatureRule { +func featureDisabledRule(feature string) inventory.FeatureRule { + flag := inventory.FeatureFlag(feature) return inventory.NewFeatureRule( - []inventory.FeatureFlag{feature}, + []inventory.FeatureFlag{flag}, func(featureAsBool inventory.FeatureResolver) bool { - return !featureAsBool(feature) + return !featureAsBool(flag) }, ) } @@ -99,15 +101,6 @@ var ( pullRequestsConsolidatedRule = featureDisabledRule(FeatureFlagPullRequestsGranular) ) -// FeatureFlagsFromStrings converts feature names from transport or CLI input. -func FeatureFlagsFromStrings(features []string) []inventory.FeatureFlag { - result := make([]inventory.FeatureFlag, len(features)) - for i, feature := range features { - result[i] = inventory.FeatureFlag(feature) - } - return result -} - // ResolveFeatureFlags computes the effective set of enabled feature flags by: // 1. Taking the user-supplied flags (from --features or X-MCP-Features) and // keeping only those present in AllowedFeatureFlags. Unknown or unsafe @@ -123,8 +116,8 @@ func FeatureFlagsFromStrings(features []string) []inventory.FeatureFlag { // 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 []inventory.FeatureFlag, insidersMode bool) map[inventory.FeatureFlag]bool { - effective := make(map[inventory.FeatureFlag]bool) +func ResolveFeatureFlags(enabledFeatures []string, insidersMode bool) map[string]bool { + effective := make(map[string]bool) for _, feature := range enabledFeatures { if slices.Contains(AllowedFeatureFlags, feature) { effective[feature] = true diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 1b01e750a4..bc0cf6b357 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -140,26 +140,26 @@ func TestResolveFeatureFlags(t *testing.T) { tests := []struct { name string - enabledFeatures []inventory.FeatureFlag + enabledFeatures []string insidersMode bool - expectedFlags []inventory.FeatureFlag - unexpectedFlags []inventory.FeatureFlag + expectedFlags []string + unexpectedFlags []string }{ { name: "no features, no insiders", enabledFeatures: nil, expectedFlags: nil, - unexpectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, + unexpectedFlags: []string{MCPAppsFeatureFlag}, }, { name: "explicit feature enabled", - enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag}, - expectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, + enabledFeatures: []string{MCPAppsFeatureFlag}, + expectedFlags: []string{MCPAppsFeatureFlag}, }, { name: "MCP Apps form deferral can be disabled directly", - enabledFeatures: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, - expectedFlags: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, + enabledFeatures: []string{MCPAppsDisableFormDeferralFeatureFlag}, + expectedFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, }, { name: "insiders mode enables insiders flags", @@ -171,50 +171,50 @@ func TestResolveFeatureFlags(t *testing.T) { name: "insiders mode does not auto-enable ifc labels", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []inventory.FeatureFlag{FeatureFlagIFCLabels}, + unexpectedFlags: []string{FeatureFlagIFCLabels}, }, { name: "insiders mode does not disable MCP Apps form deferral", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []inventory.FeatureFlag{MCPAppsDisableFormDeferralFeatureFlag}, + unexpectedFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, }, { name: "ifc_labels can be directly enabled", - enabledFeatures: []inventory.FeatureFlag{FeatureFlagIFCLabels}, - expectedFlags: []inventory.FeatureFlag{FeatureFlagIFCLabels}, + enabledFeatures: []string{FeatureFlagIFCLabels}, + expectedFlags: []string{FeatureFlagIFCLabels}, }, { name: "unknown flags are filtered out", - enabledFeatures: []inventory.FeatureFlag{"unknown_flag", "another_unknown"}, - unexpectedFlags: []inventory.FeatureFlag{"unknown_flag", "another_unknown"}, + enabledFeatures: []string{"unknown_flag", "another_unknown"}, + unexpectedFlags: []string{"unknown_flag", "another_unknown"}, }, { name: "mix of known and unknown flags", - enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag, "unknown_flag"}, - expectedFlags: []inventory.FeatureFlag{MCPAppsFeatureFlag}, - unexpectedFlags: []inventory.FeatureFlag{"unknown_flag"}, + enabledFeatures: []string{MCPAppsFeatureFlag, "unknown_flag"}, + expectedFlags: []string{MCPAppsFeatureFlag}, + unexpectedFlags: []string{"unknown_flag"}, }, { name: "user-only flags can be enabled but are not turned on by insiders", - enabledFeatures: []inventory.FeatureFlag{FeatureFlagIssuesGranular}, + enabledFeatures: []string{FeatureFlagIssuesGranular}, insidersMode: false, - expectedFlags: []inventory.FeatureFlag{FeatureFlagIssuesGranular}, + expectedFlags: []string{FeatureFlagIssuesGranular}, }, { name: "thread resolution reason can be directly enabled", - enabledFeatures: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, - expectedFlags: []inventory.FeatureFlag{FeatureFlagThreadResolutionReason}, + enabledFeatures: []string{FeatureFlagThreadResolutionReason}, + expectedFlags: []string{FeatureFlagThreadResolutionReason}, }, { name: "insiders does not enable user-only allowed flags", enabledFeatures: nil, insidersMode: true, - unexpectedFlags: []inventory.FeatureFlag{FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular}, + unexpectedFlags: []string{FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular}, }, { name: "explicit plus insiders deduplicates", - enabledFeatures: []inventory.FeatureFlag{MCPAppsFeatureFlag}, + enabledFeatures: []string{MCPAppsFeatureFlag}, insidersMode: true, expectedFlags: InsidersFeatureFlags, }, @@ -254,12 +254,12 @@ func TestThreadResolutionReasonToolVariants(t *testing.T) { }, { name: "granular flag off", - flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular}, + flags: []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagPullRequestsGranular)}, toolName: "resolve_review_thread", }, { name: "granular flag on", - flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagPullRequestsGranular), FeatureFlagThreadResolutionReason}, toolName: "resolve_review_thread", hasReason: true, }, @@ -271,7 +271,7 @@ func TestThreadResolutionReasonToolVariants(t *testing.T) { }, { name: "granular flag on GHES", - flags: []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + flags: []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagPullRequestsGranular), FeatureFlagThreadResolutionReason}, host: utils.HostTypeGHES, toolName: "resolve_review_thread", }, diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 1fe943bfea..456129a0fe 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -20,16 +20,17 @@ import ( "github.com/stretchr/testify/require" ) -func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag inventory.FeatureFlag) []inventory.ServerTool { +func granularToolsForToolset(toolsetID inventory.ToolsetID, featureFlag string) []inventory.ServerTool { + flag := inventory.FeatureFlag(featureFlag) var result []inventory.ServerTool for _, tool := range AllTools(translations.NullTranslationHelper) { features := tool.FeatureRule.Features() usesFeature := false for _, feature := range features { - usesFeature = usesFeature || feature == featureFlag + usesFeature = usesFeature || feature == flag } if tool.Toolset.ID == toolsetID && usesFeature && - tool.FeatureRule.Enabled(func(flag inventory.FeatureFlag) bool { return flag == featureFlag }) { + tool.FeatureRule.Enabled(func(feature inventory.FeatureFlag) bool { return feature == flag }) { result = append(result, tool) } } @@ -108,7 +109,7 @@ func TestIssuesGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataIssues.ID, FeatureFlagIssuesGranular) { - assert.Equal(t, []inventory.FeatureFlag{FeatureFlagIssuesGranular}, tool.FeatureRule.Features(), "tool %s", tool.Tool.Name) + assert.Equal(t, []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagIssuesGranular)}, tool.FeatureRule.Features(), "tool %s", tool.Tool.Name) } }) } @@ -144,7 +145,7 @@ func TestPullRequestsGranularToolset(t *testing.T) { t.Run("all granular tools have correct feature flag", func(t *testing.T) { for _, tool := range granularToolsForToolset(ToolsetMetadataPullRequests.ID, FeatureFlagPullRequestsGranular) { - assert.Contains(t, tool.FeatureRule.Features(), FeatureFlagPullRequestsGranular, "tool %s", tool.Tool.Name) + assert.Contains(t, tool.FeatureRule.Features(), inventory.FeatureFlag(FeatureFlagPullRequestsGranular), "tool %s", tool.Tool.Name) } }) } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 6ca5b4b101..2707dafb4b 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1833,7 +1833,7 @@ func Test_CreateIssue(t *testing.T) { serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []inventory.FeatureFlag{FeatureFlagIssuesGranular}, serverTool.FeatureRule.Features()) + require.Equal(t, []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagIssuesGranular)}, serverTool.FeatureRule.Features()) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 6a92d50a5a..1d283fa691 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1896,10 +1896,10 @@ Available methods: }) if withResolutionReason { st.FeatureRule = inventory.NewFeatureRule( - []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, FeatureFlagPullRequestsGranular}, + []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, inventory.FeatureFlag(FeatureFlagPullRequestsGranular)}, func(featureAsBool inventory.FeatureResolver) bool { return featureAsBool(FeatureFlagThreadResolutionReason) && - !featureAsBool(FeatureFlagPullRequestsGranular) + !featureAsBool(inventory.FeatureFlag(FeatureFlagPullRequestsGranular)) }, ) } else { @@ -1907,10 +1907,10 @@ Available methods: st.FeatureRule = pullRequestsConsolidatedRule } else { st.FeatureRule = inventory.NewFeatureRule( - []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, FeatureFlagPullRequestsGranular}, + []inventory.FeatureFlag{FeatureFlagThreadResolutionReason, inventory.FeatureFlag(FeatureFlagPullRequestsGranular)}, func(featureAsBool inventory.FeatureResolver) bool { return !featureAsBool(FeatureFlagThreadResolutionReason) && - !featureAsBool(FeatureFlagPullRequestsGranular) + !featureAsBool(inventory.FeatureFlag(FeatureFlagPullRequestsGranular)) }, ) } diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index e1d245a951..a32723b3c7 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -751,9 +751,9 @@ func granularResolveReviewThread(t translations.TranslationHelperFunc, withResol switch { case withResolutionReason: st.FeatureRule = inventory.NewFeatureRule( - []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagPullRequestsGranular), FeatureFlagThreadResolutionReason}, func(featureAsBool inventory.FeatureResolver) bool { - return featureAsBool(FeatureFlagPullRequestsGranular) && + return featureAsBool(inventory.FeatureFlag(FeatureFlagPullRequestsGranular)) && featureAsBool(FeatureFlagThreadResolutionReason) }, ) @@ -761,9 +761,9 @@ func granularResolveReviewThread(t translations.TranslationHelperFunc, withResol st.FeatureRule = pullRequestsGranularFeatureRule default: st.FeatureRule = inventory.NewFeatureRule( - []inventory.FeatureFlag{FeatureFlagPullRequestsGranular, FeatureFlagThreadResolutionReason}, + []inventory.FeatureFlag{inventory.FeatureFlag(FeatureFlagPullRequestsGranular), FeatureFlagThreadResolutionReason}, func(featureAsBool inventory.FeatureResolver) bool { - return featureAsBool(FeatureFlagPullRequestsGranular) && + return featureAsBool(inventory.FeatureFlag(FeatureFlagPullRequestsGranular)) && !featureAsBool(FeatureFlagThreadResolutionReason) }, ) diff --git a/pkg/github/server.go b/pkg/github/server.go index 8335ce0fea..6e9b5b7566 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -113,6 +113,7 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, // and any middleware that needs to read or modify the context should be before it. ghServer.AddReceivingMiddleware(middleware...) + ghServer.AddReceivingMiddleware(injectFeatureStateMiddleware(inv)) ghServer.AddReceivingMiddleware(InjectDepsMiddleware(deps)) ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext) @@ -138,6 +139,14 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci return ghServer, nil } +func injectFeatureStateMiddleware(inv *inventory.Inventory) mcp.Middleware { + return func(next mcp.MethodHandler) mcp.MethodHandler { + return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) { + return next(inv.WithFeatureState(ctx), method, req) + } + } +} + // ResolvedEnabledToolsets determines which toolsets should be enabled based on config. // Returns nil for "use defaults", empty slice for "none", or explicit list. func ResolvedEnabledToolsets(enabledToolsets []string, enabledTools []string) []string { diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 43037bf893..6e0f7494e7 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -194,6 +194,27 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { // is already tested in pkg/github/*_test.go. } +func TestFeatureStateMiddlewareCachesHandlerChecks(t *testing.T) { + var calls int + checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) { + calls++ + return flag == "enabled", nil + } + inv, err := NewInventory(translations.NullTranslationHelper). + WithFeatureChecker(checker). + Build() + require.NoError(t, err) + + next := func(ctx context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + assert.True(t, inventory.ResolveFeature(ctx, nil, "enabled")) + assert.True(t, inventory.ResolveFeature(ctx, nil, "enabled")) + return nil, nil + } + _, err = injectFeatureStateMiddleware(inv)(next)(context.Background(), "tools/call", nil) + require.NoError(t, err) + assert.Equal(t, 1, calls) +} + // advertisedServerCapabilities connects an in-memory client to the given server // and returns the capabilities the server advertised during initialization. func advertisedServerCapabilities(t *testing.T, server *mcp.Server) *mcp.ServerCapabilities { diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 8176b2f98e..8d568878db 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -155,19 +155,15 @@ var ( // Feature flag names for granular tool variants. // When active, consolidated tools are replaced by single-purpose granular tools. - FeatureFlagIssuesGranular inventory.FeatureFlag = "issues_granular" - FeatureFlagPullRequestsGranular inventory.FeatureFlag = "pull_requests_granular" + FeatureFlagIssuesGranular = "issues_granular" + FeatureFlagPullRequestsGranular = "pull_requests_granular" ) // HeaderAllowedFeatureFlags returns the feature flags that clients may enable via // the X-MCP-Features header. It delegates to AllowedFeatureFlags as the single // source of truth. func HeaderAllowedFeatureFlags() []string { - flags := make([]string, len(AllowedFeatureFlags)) - for i, flag := range AllowedFeatureFlags { - flags[i] = string(flag) - } - return flags + return slices.Clone(AllowedFeatureFlags) } var ( diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index bbc98558ab..9d13c1aa11 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -159,7 +159,7 @@ func TestNoDuplicateToolNames(t *testing.T) { func TestMCPAppsFeatureFlagMatchesInventory(t *testing.T) { inv, err := NewInventory(stubTranslation).Build() require.NoError(t, err) - assert.Contains(t, inv.RequiredFeatures(), MCPAppsFeatureFlag) + assert.Contains(t, inv.RequiredFeatures(), inventory.FeatureFlag(MCPAppsFeatureFlag)) } // TestNoDuplicateResourceNames ensures all resources have unique names diff --git a/pkg/http/handler.go b/pkg/http/handler.go index f8c71e3d26..fc8fc045f6 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -40,7 +40,6 @@ type Handler struct { inventoryFactoryFunc InventoryFactoryFunc oauthCfg *oauth.Config scopeFetcher scopes.FetcherInterface - featureChecker inventory.FeatureFlagChecker schemaCache *mcp.SchemaCache } @@ -127,7 +126,6 @@ func NewHTTPMcpHandler( inventoryFactoryFunc: inventoryFactory, oauthCfg: opts.OAuthConfig, scopeFetcher: scopeFetcher, - featureChecker: opts.FeatureChecker, schemaCache: schemaCache, } } @@ -216,7 +214,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if methodInfo, ok := ghcontext.MCPMethod(r.Context()); ok && methodInfo != nil { invToUse = inv.ForMCPRequest(methodInfo.Method, methodInfo.ItemName) } - r = r.WithContext(inventory.WithResolvedFeatures(r.Context(), h.featureChecker, invToUse.RequiredFeatures())) + r = r.WithContext(invToUse.WithResolvedFeatures(r.Context())) ghServer, err := h.githubMcpServerFactory(r, h.deps, invToUse, &github.MCPServerConfig{ Version: h.config.Version, diff --git a/pkg/http/server.go b/pkg/http/server.go index 60f509953e..55317bd50f 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -318,7 +318,7 @@ func createHTTPFeatureChecker(enabledFeatures []string, insidersMode bool) inven features = append(features, enabledFeatures...) features = append(features, headerFeatures...) - effective := github.ResolveFeatureFlags(github.FeatureFlagsFromStrings(features), insidersMode || ghcontext.IsInsidersMode(ctx)) - return effective[flag], nil + effective := github.ResolveFeatureFlags(features, insidersMode || ghcontext.IsInsidersMode(ctx)) + return effective[string(flag)], nil } } diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go index 8a94f9c4f9..af27c9976e 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -312,41 +312,41 @@ func TestInitGlobalToolScopeMapUsesHost(t *testing.T) { func TestCreateHTTPFeatureChecker(t *testing.T) { tests := []struct { name string - staticFeatures []inventory.FeatureFlag + staticFeatures []string staticInsiders bool - flagName inventory.FeatureFlag - headerFeatures []inventory.FeatureFlag + flagName string + headerFeatures []string insidersMode bool wantEnabled bool }{ { name: "allowed issues_granular flag accepted from header", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular}, + headerFeatures: []string{github.FeatureFlagIssuesGranular}, wantEnabled: true, }, { name: "allowed pull_requests_granular flag accepted from header", flagName: github.FeatureFlagPullRequestsGranular, - headerFeatures: []inventory.FeatureFlag{github.FeatureFlagPullRequestsGranular}, + headerFeatures: []string{github.FeatureFlagPullRequestsGranular}, wantEnabled: true, }, { name: "MCP Apps flag accepted from header", flagName: github.MCPAppsFeatureFlag, - headerFeatures: []inventory.FeatureFlag{github.MCPAppsFeatureFlag}, + headerFeatures: []string{github.MCPAppsFeatureFlag}, wantEnabled: true, }, { name: "MCP Apps form deferral opt-out accepted from header", flagName: github.MCPAppsDisableFormDeferralFeatureFlag, - headerFeatures: []inventory.FeatureFlag{github.MCPAppsDisableFormDeferralFeatureFlag}, + headerFeatures: []string{github.MCPAppsDisableFormDeferralFeatureFlag}, wantEnabled: true, }, { name: "unknown flag in header is ignored", flagName: "unknown_flag", - headerFeatures: []inventory.FeatureFlag{"unknown_flag"}, + headerFeatures: []string{"unknown_flag"}, wantEnabled: false, }, { @@ -358,19 +358,19 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { { name: "allowed flag with different flag in header returns false", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []inventory.FeatureFlag{github.FeatureFlagPullRequestsGranular}, + headerFeatures: []string{github.FeatureFlagPullRequestsGranular}, wantEnabled: false, }, { name: "multiple allowed flags in header", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular, github.FeatureFlagPullRequestsGranular}, + headerFeatures: []string{github.FeatureFlagIssuesGranular, github.FeatureFlagPullRequestsGranular}, wantEnabled: true, }, { name: "empty header features", flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []inventory.FeatureFlag{}, + headerFeatures: []string{}, wantEnabled: false, }, { @@ -387,15 +387,15 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { }, { name: "static feature is enabled without header", - staticFeatures: []inventory.FeatureFlag{github.FeatureFlagCSVOutput}, + staticFeatures: []string{github.FeatureFlagCSVOutput}, flagName: github.FeatureFlagCSVOutput, wantEnabled: true, }, { name: "static features combine with header features", - staticFeatures: []inventory.FeatureFlag{github.FeatureFlagCSVOutput}, + staticFeatures: []string{github.FeatureFlagCSVOutput}, flagName: github.FeatureFlagIssuesGranular, - headerFeatures: []inventory.FeatureFlag{github.FeatureFlagIssuesGranular}, + headerFeatures: []string{github.FeatureFlagIssuesGranular}, wantEnabled: true, }, { @@ -420,16 +420,16 @@ func TestCreateHTTPFeatureChecker(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - checker := createHTTPFeatureChecker(featureFlagStrings(tt.staticFeatures), tt.staticInsiders) + checker := createHTTPFeatureChecker(tt.staticFeatures, tt.staticInsiders) ctx := context.Background() if len(tt.headerFeatures) > 0 { - ctx = ghcontext.WithHeaderFeatures(ctx, featureFlagStrings(tt.headerFeatures)) + ctx = ghcontext.WithHeaderFeatures(ctx, tt.headerFeatures) } if tt.insidersMode { ctx = ghcontext.WithInsidersMode(ctx, true) } - enabled, err := checker(ctx, tt.flagName) + enabled, err := checker(ctx, inventory.FeatureFlag(tt.flagName)) require.NoError(t, err) assert.Equal(t, tt.wantEnabled, enabled) }) @@ -514,15 +514,7 @@ func TestConfigureRequestState(t *testing.T) { func TestHeaderAllowedFeatureFlagsMatchesAllowed(t *testing.T) { // Ensure HeaderAllowedFeatureFlags delegates to AllowedFeatureFlags allowed := github.HeaderAllowedFeatureFlags() - assert.Equal(t, featureFlagStrings(github.AllowedFeatureFlags), allowed, + assert.Equal(t, github.AllowedFeatureFlags, allowed, "HeaderAllowedFeatureFlags() should match AllowedFeatureFlags") assert.NotEmpty(t, allowed, "AllowedFeatureFlags should not be empty") } - -func featureFlagStrings(flags []inventory.FeatureFlag) []string { - result := make([]string, len(flags)) - for i, flag := range flags { - result[i] = string(flag) - } - return result -} diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index 388a0640cb..b9133360a9 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -71,14 +71,16 @@ func (r FeatureRule) Enabled(featureAsBool FeatureResolver) bool { } var undeclared FeatureFlag + usedUndeclared := false enabled := r.predicate(func(feature FeatureFlag) bool { if _, ok := r.featureSet[feature]; !ok { undeclared = feature + usedUndeclared = true return false } return featureAsBool(feature) }) - if undeclared != "" { + if usedUndeclared { fmt.Fprintf(os.Stderr, "Feature rule used undeclared feature %q\n", undeclared) return false } diff --git a/pkg/inventory/features_test.go b/pkg/inventory/features_test.go index 6d7bba30d9..74dd9183a8 100644 --- a/pkg/inventory/features_test.go +++ b/pkg/inventory/features_test.go @@ -47,6 +47,17 @@ func TestFeatureRuleFailsClosedForUndeclaredFeature(t *testing.T) { assert.False(t, rule.Enabled(func(FeatureFlag) bool { return true })) } +func TestFeatureRuleFailsClosedForEmptyFeature(t *testing.T) { + rule := NewFeatureRule( + []FeatureFlag{"declared"}, + func(featureAsBool FeatureResolver) bool { + return !featureAsBool("") + }, + ) + + assert.False(t, rule.Enabled(func(FeatureFlag) bool { return true })) +} + func TestResolvedFeaturesDeduplicateAndCacheChecks(t *testing.T) { calls := make(map[FeatureFlag]int) checker := func(_ context.Context, flag FeatureFlag) (bool, error) { diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 5e658b5c9b..be571c73e7 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -242,6 +242,18 @@ func (r *Inventory) RequiredFeatures() []FeatureFlag { return result } +// WithFeatureState installs request-owned feature state without resolving any +// flags up front. Handler-only checks are resolved lazily and cached. +func (r *Inventory) WithFeatureState(ctx context.Context) context.Context { + return WithResolvedFeatures(ctx, r.featureChecker, nil) +} + +// WithResolvedFeatures installs request-owned feature state and resolves every +// feature required by the current inventory through its own checker. +func (r *Inventory) WithResolvedFeatures(ctx context.Context) context.Context { + return WithResolvedFeatures(ctx, r.featureChecker, r.RequiredFeatures()) +} + func (r *Inventory) usesMCPAppsMetadata() bool { for i := range r.tools { for _, key := range mcpAppsMetaKeys {