From cee49f5bda911e90fdf6134685dba95c4435be0c Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 27 Aug 2026 11:49:35 +0200 Subject: [PATCH 01/15] 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 | 29 +++- 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, 746 insertions(+), 375 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 fe72955b08..f917d48f07 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -35,15 +35,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. diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 6191857ac8..88c8d7ac53 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -207,6 +207,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. @@ -219,5 +226,6 @@ Insiders is a **meta feature flag** — the same shape as `default` or `all` for 2. Add it to `AllowedFeatureFlags` if end users should be able to opt in via `--features`, `X-MCP-Features`, or the `features` URL query parameter. 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 a388f30d6b..ea2cb39e8d 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -1,48 +1,52 @@ 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, X-MCP-Features HTTP header, or the // features URL query parameter. // 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, @@ -59,7 +63,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, @@ -71,6 +75,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 HTTP request // configuration) and @@ -87,11 +125,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 fd7ea36873..9450476b19 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 e8b4cd2c13..c88d92edd3 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 925e7d55a0..448fa47425 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1169,7 +1169,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultText(string(r)), nil, nil }) - st.FeatureFlagDisable = []string{FeatureFlagPullRequestsGranular} + st.FeatureRule = pullRequestsConsolidatedRule return st } @@ -1907,12 +1907,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 @@ -2459,7 +2471,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 8dfa19b4a2..326bea51c6 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -3030,7 +3030,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 71b04faa3e..d88c13e7a5 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -5971,7 +5971,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 ca46deadd2..e4ca6d26cd 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 // through the X-MCP-Features header or features URL query parameter. 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 cdc12348ae..4687f4cea8 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -180,7 +180,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 } } @@ -196,6 +196,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 406f845897..6fdac08617 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 } @@ -370,9 +377,9 @@ func TestHTTPHandlerRoutes(t *testing.T) { var capturedCtx context.Context // Match the production allowlist and insiders expansion behavior. - featureChecker := func(ctx context.Context, flag string) (bool, error) { + featureChecker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { effective := github.ResolveFeatureFlags( - ghcontext.GetHeaderFeatures(ctx), + github.FeatureFlagsFromStrings(ghcontext.GetHeaderFeatures(ctx)), ghcontext.IsInsidersMode(ctx), ) return effective[flag], nil @@ -579,8 +586,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") @@ -763,7 +770,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 8a3a305e49..0b95c4907f 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 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) { requestFeatures := ghcontext.GetHeaderFeatures(ctx) features := make([]string, 0, len(enabledFeatures)+len(requestFeatures)) features = append(features, enabledFeatures...) features = append(features, requestFeatures...) - 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 500bb40611..226bab262b 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -355,41 +355,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, }, { @@ -401,19 +401,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, }, { @@ -430,15 +430,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, }, { @@ -463,10 +463,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) @@ -557,7 +557,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 0e9150925cd7ae110b09d40048dfb445c52da3c0 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 27 Aug 2026 12:06:46 +0200 Subject: [PATCH 02/15] 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 ea2cb39e8d..94f335ebe5 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -7,46 +7,46 @@ 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, X-MCP-Features HTTP header, or the // features URL query parameter. // 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, @@ -63,7 +63,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, @@ -75,20 +75,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) }, ) } @@ -100,15 +102,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 HTTP request // configuration) and @@ -125,8 +118,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 c88d92edd3..a894641ddf 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 448fa47425..5cee8b3231 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1908,10 +1908,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 { @@ -1919,10 +1919,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 e4ca6d26cd..ca46deadd2 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 // through the X-MCP-Features header or features URL query parameter. 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 4687f4cea8..31b39d1a45 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -199,7 +199,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 0b95c4907f..7c484bbd6b 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, requestFeatures...) - 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 226bab262b..da4891ffa1 100644 --- a/pkg/http/server_test.go +++ b/pkg/http/server_test.go @@ -355,41 +355,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, }, { @@ -401,19 +401,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, }, { @@ -430,15 +430,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, }, { @@ -463,16 +463,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) }) @@ -557,15 +557,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 { From b33585e05bea0b1ecde841ea17c980fa86a76d06 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 1 Sep 2026 10:27:46 +0200 Subject: [PATCH 03/15] fix(features): harden request resolution Make feature checks reentrant and single-flight, validate rule declarations across short-circuit paths, cache inventory feature metadata, and codify the HTTP context boundary used by the remote server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- docs/feature-flags.md | 13 ++++ pkg/github/dependencies.go | 10 ++- pkg/http/handler.go | 8 ++ pkg/http/handler_test.go | 68 +++++++++++++++++ pkg/inventory/builder.go | 2 + pkg/inventory/features.go | 132 ++++++++++++++++++++++++--------- pkg/inventory/features_test.go | 121 +++++++++++++++++++++++++----- pkg/inventory/filters.go | 10 ++- pkg/inventory/registry.go | 98 ++++++++++++------------ pkg/inventory/registry_test.go | 22 ++++++ 10 files changed, 377 insertions(+), 107 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index f917d48f07..e998527e85 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -55,6 +55,19 @@ 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`. +Feature predicates are pure and may depend only on their resolver. Construction +validates every combination of up to 16 declared flags, so an undeclared lookup +fails immediately even when ordinary evaluation would short-circuit that +branch. + +The inventory's checker owns request feature state. Once installed, that state +is authoritative; a checker stored on tool dependencies is used only as a +fallback when handlers are invoked directly without request state. HTTP +availability is resolved after outer HTTP middleware and the inventory factory +run, but before MCP receiving middleware, because the tool set must be known +before constructing the MCP server. Handler-only lazy checks use the live +tool-call context. + --- ## Tools affected by each flag diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 9152e073a2..8ebe2db042 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -203,9 +203,9 @@ func (d BaseDeps) Metrics(ctx context.Context) metrics.Metrics { // GetRequestStateSealer implements RequestStateSealerProvider. func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSealer } -// 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. +// IsFeatureEnabled checks if a feature flag is enabled. Request feature state +// is authoritative when present; the dependency checker is a fallback for +// direct handler invocation. Empty names and checker errors resolve false. func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { return inventory.ResolveFeature(ctx, d.featureChecker, flag) } @@ -483,7 +483,9 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics { return d.obsv.Metrics(ctx) } -// IsFeatureEnabled checks if a feature flag is enabled. +// IsFeatureEnabled checks if a feature flag is enabled. Request feature state +// is authoritative when present; the dependency checker is a fallback for +// direct handler invocation. func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { return inventory.ResolveFeature(ctx, d.featureChecker, flag) } diff --git a/pkg/http/handler.go b/pkg/http/handler.go index fc8fc045f6..95f502b320 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -23,6 +23,11 @@ import ( const subscriptionsListenMethod = "subscriptions/listen" +// InventoryFactoryFunc builds the inventory for one HTTP request. All context +// values required by its feature checker must be installed before this runs: +// feature availability is resolved immediately afterward, before MCP receiving +// middleware can run. Handler-only lazy checks still see receiving-middleware +// context. type InventoryFactoryFunc func(r *http.Request) (*inventory.Inventory, error) // GitHubMCPServerFactoryFunc is a function type for creating a new MCP Server instance. @@ -214,6 +219,9 @@ 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) } + // Tool registration must know availability before the MCP server exists. + // Remote consumers install user identity in HTTP middleware before the + // inventory factory, so their per-user checker has its full context here. r = r.WithContext(invToUse.WithResolvedFeatures(r.Context())) ghServer, err := h.githubMcpServerFactory(r, h.deps, invToUse, &github.MCPServerConfig{ diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 6fdac08617..4a5e3dce29 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -1046,6 +1046,74 @@ func TestCrossOriginProtection(t *testing.T) { } } +func TestFeatureResolutionUsesOuterHTTPContext(t *testing.T) { + type userContextKey struct{} + const ( + userValue = "remote-user" + featureFlag = inventory.FeatureFlag("remote-feature") + ) + + var checkerCalls int + tool := mockTool("feature_tool", "test", true) + tool.FeatureRule = inventory.NewFeatureRule( + []inventory.FeatureFlag{featureFlag}, + func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(featureFlag) + }, + ) + inventoryFactory := func(_ *http.Request) (*inventory.Inventory, error) { + checker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { + checkerCalls++ + return flag == featureFlag && ctx.Value(userContextKey{}) == userValue, nil + } + return inventory.NewBuilder(). + SetTools([]inventory.ServerTool{tool}). + WithToolsets([]string{"all"}). + WithFeatureChecker(checker). + Build() + } + + apiHost, err := utils.NewAPIHost("https://api.github.com") + require.NoError(t, err) + handler := NewHTTPMcpHandler( + context.Background(), + &ServerConfig{Version: "test"}, + nil, + translations.NullTranslationHelper, + slog.Default(), + apiHost, + WithInventoryFactory(inventoryFactory), + WithGitHubMCPServerFactory(func(r *http.Request, _ github.ToolDependencies, inv *inventory.Inventory, _ *github.MCPServerConfig) (*mcp.Server, error) { + assert.True(t, inventory.ResolveFeature(r.Context(), nil, featureFlag)) + require.Len(t, inv.AvailableTools(r.Context()), 1) + return mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.0.1"}, nil), nil + }), + WithScopeFetcher(allScopesFetcher{}), + ) + + router := chi.NewRouter() + router.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), userContextKey{}, userValue))) + }) + }) + handler.RegisterMiddleware(router) + handler.RegisterRoutes(router) + + body := `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{"_meta":{"io.modelcontextprotocol/protocolVersion":"2026-07-28","io.modelcontextprotocol/clientInfo":{"name":"test","version":"1.0.0"},"io.modelcontextprotocol/clientCapabilities":{}}}}` + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set(headers.ContentTypeHeader, headers.ContentTypeJSON) + req.Header.Set(headers.AcceptHeader, strings.Join([]string{headers.ContentTypeJSON, headers.ContentTypeEventStream}, ", ")) + req.Header.Set("Mcp-Protocol-Version", "2026-07-28") + req.Header.Set("Mcp-Method", "tools/list") + req.Header.Set(headers.AuthorizationHeader, "ghs_test-token") + + recorder := httptest.NewRecorder() + router.ServeHTTP(recorder, req) + require.Equal(t, http.StatusOK, recorder.Code, "response body: %s", recorder.Body.String()) + assert.Equal(t, 1, checkerCalls) +} + func TestHTTPToolMinimumProtocolVersion(t *testing.T) { apiHost, err := utils.NewAPIHost("https://api.github.com") require.NoError(t, err) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 20cd9f5838..1937b0c6df 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -255,6 +255,8 @@ func (b *Builder) Build() (*Inventory, error) { } } + r.cacheFeatureMetadata() + if b.generateInstructions { r.instructions = generateInstructions(r) } diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index b9133360a9..21b7263521 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -8,17 +8,22 @@ import ( "sync" ) +const maxFeatureRuleFlags = 16 + // FeatureFlag identifies a feature consistently across inventory consumers. type FeatureFlag string -// FeatureFlagChecker resolves one feature flag for the current request. +// FeatureFlagChecker resolves one feature flag for the current request. Every +// context value needed for availability checks must be installed before the +// inventory is resolved. Handler-only checks receive the live tool-call context. 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. +// FeaturePredicate determines whether an inventory item is available. Predicates +// must be pure: their result may depend only on calls to the supplied resolver. type FeaturePredicate func(featureAsBool FeatureResolver) bool // FeatureRule declares the feature flags used by an availability predicate. @@ -44,11 +49,33 @@ func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureR featureSet[feature] = struct{}{} declared = append(declared, feature) } - return FeatureRule{ + rule := FeatureRule{ features: declared, featureSet: featureSet, predicate: predicate, } + rule.validate() + return rule +} + +func (r FeatureRule) validate() { + if r.predicate == nil { + return + } + if len(r.features) > maxFeatureRuleFlags { + panic(fmt.Sprintf("feature rule declares %d flags; maximum is %d", len(r.features), maxFeatureRuleFlags)) + } + + for assignment := range 1 << len(r.features) { + r.evaluate(func(feature FeatureFlag) bool { + for i, declared := range r.features { + if feature == declared { + return assignment&(1< Date: Tue, 1 Sep 2026 15:15:40 +0200 Subject: [PATCH 04/15] test(features): preserve query flag coverage after rebase Resolve the feature-query test against the final string-compatible flag API. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/http/handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 4a5e3dce29..1d32b8c43b 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -379,10 +379,10 @@ func TestHTTPHandlerRoutes(t *testing.T) { // Match the production allowlist and insiders expansion behavior. featureChecker := func(ctx context.Context, flag inventory.FeatureFlag) (bool, error) { effective := github.ResolveFeatureFlags( - github.FeatureFlagsFromStrings(ghcontext.GetHeaderFeatures(ctx)), + ghcontext.GetHeaderFeatures(ctx), ghcontext.IsInsidersMode(ctx), ) - return effective[flag], nil + return effective[string(flag)], nil } apiHost, err := utils.NewAPIHost("https://api.github.com") From af10d2059c8f32725aef5515f9ba804858c0eb9c Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 1 Sep 2026 16:42:05 +0200 Subject: [PATCH 05/15] fix(features): close cycle and ownership gaps Preserve the public string dependency API, detect concurrent resolution cycles without blocking, and isolate cached feature metadata from caller mutation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/github/csv_output_test.go | 2 +- pkg/github/dependencies.go | 10 +-- pkg/github/feature_flags_test.go | 2 +- pkg/github/server_test.go | 2 +- pkg/inventory/builder.go | 37 +++++++++-- pkg/inventory/features.go | 103 +++++++++++++++++++++++++++---- pkg/inventory/features_test.go | 64 ++++++++++++++++++- pkg/inventory/filters.go | 6 +- pkg/inventory/registry.go | 2 +- pkg/inventory/registry_test.go | 60 ++++++++++++++++++ 10 files changed, 258 insertions(+), 30 deletions(-) diff --git a/pkg/github/csv_output_test.go b/pkg/github/csv_output_test.go index f9bb0e28c5..44dbff6280 100644 --- a/pkg/github/csv_output_test.go +++ b/pkg/github/csv_output_test.go @@ -366,7 +366,7 @@ type csvOutputTestDeps struct { csvOn bool } -func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag inventory.FeatureFlag) bool { +func (d csvOutputTestDeps) IsFeatureEnabled(_ context.Context, flag string) bool { return flag == FeatureFlagCSVOutput && d.csvOn } diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 8ebe2db042..333b1d9c07 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -94,7 +94,7 @@ type ToolDependencies interface { GetContentWindowSize() int // IsFeatureEnabled checks if a feature flag is enabled. - IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool + IsFeatureEnabled(ctx context.Context, flag string) bool // Logger returns the structured logger, optionally enriched with // request-scoped data from ctx. Integrators provide their own slog.Handler @@ -206,8 +206,8 @@ func (d BaseDeps) GetRequestStateSealer() RequestStateSealer { return d.StateSea // IsFeatureEnabled checks if a feature flag is enabled. Request feature state // is authoritative when present; the dependency checker is a fallback for // direct handler invocation. Empty names and checker errors resolve false. -func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { - return inventory.ResolveFeature(ctx, d.featureChecker, flag) +func (d BaseDeps) IsFeatureEnabled(ctx context.Context, flag string) bool { + return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag)) } // NewTool creates a ServerTool that retrieves ToolDependencies from context at call time. @@ -486,6 +486,6 @@ func (d *RequestDeps) Metrics(ctx context.Context) metrics.Metrics { // IsFeatureEnabled checks if a feature flag is enabled. Request feature state // is authoritative when present; the dependency checker is a fallback for // direct handler invocation. -func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag inventory.FeatureFlag) bool { - return inventory.ResolveFeature(ctx, d.featureChecker, flag) +func (d *RequestDeps) IsFeatureEnabled(ctx context.Context, flag string) bool { + return inventory.ResolveFeature(ctx, d.featureChecker, inventory.FeatureFlag(flag)) } diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index bc0cf6b357..30282fab13 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -47,7 +47,7 @@ func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool { // Check feature flag to determine greeting style greeting := "Hello, world!" - if deps.IsFeatureEnabled(ctx, RemoteMCPEnthusiasticGreeting) { + if deps.IsFeatureEnabled(ctx, string(RemoteMCPEnthusiasticGreeting)) { greeting += " Welcome to the future of MCP! 🎉" } diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index 6e0f7494e7..e81247ac99 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -65,7 +65,7 @@ func (s stubDeps) GetRepoAccessCache(_ context.Context) (*lockdown.RepoAccessCac 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 { +func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } func (s stubDeps) Logger(_ context.Context) *slog.Logger { diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 1937b0c6df..f75013affe 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -23,7 +23,9 @@ const mcpAppsFeatureFlag FeatureFlag = "remote_mcp_ui_apps" // Returns true if the tool should be included, false to exclude it. type ToolFilter func(ctx context.Context, tool *ServerTool) (bool, error) -// Builder builds a Registry with the specified configuration. +// Builder builds a Registry with the specified configuration. SetTools, +// SetResources, and SetPrompts copy the feature and metadata state retained by +// the inventory. // Use NewBuilder to create a builder, chain configuration methods, // then call Build() to create the final inventory. // @@ -65,22 +67,49 @@ func NewBuilder() *Builder { // SetTools sets the tools for the inventory. Returns self for chaining. func (b *Builder) SetTools(tools []ServerTool) *Builder { - b.tools = tools + b.tools = slices.Clone(tools) + for i := range b.tools { + b.tools[i] = cloneServerTool(b.tools[i]) + } return b } // SetResources sets the resource templates for the inventory. Returns self for chaining. func (b *Builder) SetResources(resources []ServerResourceTemplate) *Builder { - b.resourceTemplates = resources + b.resourceTemplates = slices.Clone(resources) + for i := range b.resourceTemplates { + b.resourceTemplates[i] = cloneResourceTemplate(b.resourceTemplates[i]) + } return b } // SetPrompts sets the prompts for the inventory. Returns self for chaining. func (b *Builder) SetPrompts(prompts []ServerPrompt) *Builder { - b.prompts = prompts + b.prompts = slices.Clone(prompts) + for i := range b.prompts { + b.prompts[i] = clonePrompt(b.prompts[i]) + } return b } +func cloneServerTool(tool ServerTool) ServerTool { + tool.Tool.Meta = maps.Clone(tool.Tool.Meta) + tool.FeatureRule = tool.FeatureRule.clone() + return tool +} + +func cloneResourceTemplate(resource ServerResourceTemplate) ServerResourceTemplate { + resource.Template.Meta = maps.Clone(resource.Template.Meta) + resource.FeatureRule = resource.FeatureRule.clone() + return resource +} + +func clonePrompt(prompt ServerPrompt) ServerPrompt { + prompt.Prompt.Meta = maps.Clone(prompt.Prompt.Meta) + prompt.FeatureRule = prompt.FeatureRule.clone() + return prompt +} + // WithDeprecatedAliases adds deprecated tool name aliases that map to canonical names. // Returns self for chaining. func (b *Builder) WithDeprecatedAliases(aliases map[string]string) *Builder { diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index 21b7263521..5cdeeed1a9 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -3,6 +3,7 @@ package inventory import ( "context" "fmt" + "maps" "os" "slices" "sync" @@ -35,6 +36,14 @@ type FeatureRule struct { predicate FeaturePredicate } +func (r FeatureRule) clone() FeatureRule { + return FeatureRule{ + features: slices.Clone(r.features), + featureSet: maps.Clone(r.featureSet), + predicate: r.predicate, + } +} + // NewFeatureRule creates an availability rule over the supplied feature flags. func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureRule { declared := make([]FeatureFlag, 0, len(features)) @@ -124,22 +133,25 @@ type featureState struct { mu sync.Mutex results map[FeatureFlag]*featureResult + waiting map[FeatureFlag]map[FeatureFlag]int } type featureResult struct { ready chan struct{} enabled bool + failed bool + done bool } type resolvingFeature struct { - flag FeatureFlag - parent *resolvingFeature + flag FeatureFlag } func newFeatureState(checker FeatureFlagChecker) *featureState { return &featureState{ checker: checker, results: make(map[FeatureFlag]*featureResult), + waiting: make(map[FeatureFlag]map[FeatureFlag]int), } } @@ -148,21 +160,34 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool { return false } - for current := resolvingFeatureFromContext(ctx); current != nil; current = current.parent { - if current.flag == feature { - fmt.Fprintf(os.Stderr, "Feature flag resolution cycle detected for %q\n", feature) - return false - } - } - + owner := resolvingFeatureFromContext(ctx) s.mu.Lock() result, found := s.results[feature] + if found && result.done { + enabled := result.enabled + s.mu.Unlock() + return enabled + } if !found { result = &featureResult{ready: make(chan struct{})} s.results[feature] = result } + if owner != nil { + if path := s.pathLocked(feature, owner.flag, nil); path != nil { + s.failCycleLocked(owner.flag, path) + s.mu.Unlock() + return false + } + if s.waiting[owner.flag] == nil { + s.waiting[owner.flag] = make(map[FeatureFlag]int) + } + s.waiting[owner.flag][feature]++ + } s.mu.Unlock() + if owner != nil { + defer s.clearWait(owner.flag, feature) + } if found { select { case <-result.ready: @@ -175,25 +200,77 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool { completed := false defer func() { if !completed { + s.mu.Lock() + result.failed = true + result.done = true close(result.ready) + s.mu.Unlock() } }() - resolutionCtx := context.WithValue(ctx, resolvingFeatureContextKey{}, &resolvingFeature{ - flag: feature, - parent: resolvingFeatureFromContext(ctx), - }) + resolutionCtx := context.WithValue(ctx, resolvingFeatureContextKey{}, &resolvingFeature{flag: feature}) enabled, err := s.checker(resolutionCtx, feature) if err != nil { fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", feature, err) enabled = false } + s.mu.Lock() + if result.failed { + enabled = false + } result.enabled = enabled + result.done = true completed = true close(result.ready) + s.mu.Unlock() return enabled } +func (s *featureState) pathLocked(current, target FeatureFlag, seen map[FeatureFlag]bool) []FeatureFlag { + if current == target { + return []FeatureFlag{current} + } + if seen == nil { + seen = make(map[FeatureFlag]bool) + } + if seen[current] { + return nil + } + seen[current] = true + for next := range s.waiting[current] { + if path := s.pathLocked(next, target, seen); path != nil { + return append([]FeatureFlag{current}, path...) + } + } + return nil +} + +func (s *featureState) failCycleLocked(owner FeatureFlag, path []FeatureFlag) { + if result := s.results[owner]; result != nil { + result.failed = true + } + for _, feature := range path { + if result := s.results[feature]; result != nil { + result.failed = true + } + } + fmt.Fprintf(os.Stderr, "Feature flag resolution cycle detected for %q\n", path[0]) +} + +func (s *featureState) clearWait(owner, target FeatureFlag) { + s.mu.Lock() + if targets := s.waiting[owner]; targets != nil { + targets[target]-- + if targets[target] == 0 { + delete(targets, target) + } + } + if len(s.waiting[owner]) == 0 { + delete(s.waiting, owner) + } + s.mu.Unlock() +} + func resolvingFeatureFromContext(ctx context.Context) *resolvingFeature { feature, _ := ctx.Value(resolvingFeatureContextKey{}).(*resolvingFeature) return feature diff --git a/pkg/inventory/features_test.go b/pkg/inventory/features_test.go index a352c07823..4608812be3 100644 --- a/pkg/inventory/features_test.go +++ b/pkg/inventory/features_test.go @@ -131,7 +131,7 @@ func TestFeatureResolutionIsReentrantAcrossFlags(t *testing.T) { assert.True(t, ResolveFeature(ctx, nil, "meta")) } -func TestFeatureResolutionCycleFailsClosed(t *testing.T) { +func TestDirectFeatureResolutionCycleFailsClosed(t *testing.T) { var checker FeatureFlagChecker checker = func(ctx context.Context, flag FeatureFlag) (bool, error) { return ResolveFeature(ctx, checker, flag), nil @@ -141,6 +141,34 @@ func TestFeatureResolutionCycleFailsClosed(t *testing.T) { assert.False(t, ResolveFeature(ctx, nil, "cycle")) } +func TestSelfNegatingFeatureResolutionCycleFailsClosed(t *testing.T) { + var checker FeatureFlagChecker + checker = func(ctx context.Context, flag FeatureFlag) (bool, error) { + return !ResolveFeature(ctx, checker, flag), nil + } + + ctx := WithResolvedFeatures(context.Background(), checker, nil) + assert.False(t, ResolveFeature(ctx, nil, "cycle")) +} + +func TestMutualFeatureResolutionCycleFailsClosed(t *testing.T) { + var checker FeatureFlagChecker + checker = func(ctx context.Context, flag FeatureFlag) (bool, error) { + switch flag { + case "a": + return !ResolveFeature(ctx, checker, "b"), nil + case "b": + return !ResolveFeature(ctx, checker, "a"), nil + default: + return false, nil + } + } + + ctx := WithResolvedFeatures(context.Background(), checker, nil) + assert.False(t, ResolveFeature(ctx, nil, "a")) + assert.False(t, ResolveFeature(ctx, nil, "b")) +} + func TestConcurrentFeatureResolutionIsDeduplicated(t *testing.T) { var ( calls int @@ -177,6 +205,40 @@ func TestConcurrentFeatureResolutionIsDeduplicated(t *testing.T) { callsMu.Unlock() } +func TestConcurrentCrossFeatureCycleFailsClosed(t *testing.T) { + startedA := make(chan struct{}) + startedB := make(chan struct{}) + var checker FeatureFlagChecker + checker = func(ctx context.Context, flag FeatureFlag) (bool, error) { + switch flag { + case "a": + close(startedA) + <-startedB + return !ResolveFeature(ctx, checker, "b"), nil + case "b": + close(startedB) + <-startedA + return !ResolveFeature(ctx, checker, "a"), nil + default: + return false, nil + } + } + + ctx := WithResolvedFeatures(context.Background(), checker, nil) + results := make(chan bool, 2) + go func() { results <- ResolveFeature(ctx, nil, "a") }() + go func() { results <- ResolveFeature(ctx, nil, "b") }() + + for range 2 { + select { + case result := <-results: + assert.False(t, result) + case <-time.After(time.Second): + t.Fatal("concurrent feature cycle deadlocked") + } + } +} + func TestContextFeatureStateTakesPrecedence(t *testing.T) { var fallbackCalls int stateChecker := func(context.Context, FeatureFlag) (bool, error) { diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 4ab4d3c0b0..1209910d59 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -106,7 +106,7 @@ func (r *Inventory) availableTools(ctx context.Context) []ServerTool { for i := range r.tools { tool := &r.tools[i] if r.isToolEnabled(ctx, tool, featureAsBool) { - result = append(result, *tool) + result = append(result, cloneServerTool(*tool)) } } @@ -136,7 +136,7 @@ func (r *Inventory) AvailableResourceTemplates(ctx context.Context) []ServerReso continue } if r.isToolsetEnabled(res.Toolset.ID) { - result = append(result, *res) + result = append(result, cloneResourceTemplate(*res)) } } @@ -166,7 +166,7 @@ func (r *Inventory) AvailablePrompts(ctx context.Context) []ServerPrompt { continue } if r.isToolsetEnabled(prompt.Toolset.ID) { - result = append(result, *prompt) + result = append(result, clonePrompt(*prompt)) } } diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index ab0c6675ee..5dc2b081e4 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -53,7 +53,7 @@ type Inventory struct { // Takes context and flag name, returns (enabled, error). If error, log and treat as false. // If checker is nil, all flag checks return false. featureChecker FeatureFlagChecker - // Feature metadata is derived once from the immutable inventory contents. + // Feature metadata is derived once from builder-owned inventory contents. toolFeatures []FeatureFlag resourceTemplateFeatures []FeatureFlag promptFeatures []FeatureFlag diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 077af0ad12..5879808b2c 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1243,6 +1243,66 @@ func TestFeatureMetadataIsCachedAndNarrowed(t *testing.T) { require.Equal(t, []FeatureFlag{"changed"}, narrowed.RequiredFeatures()) } +func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { + toolRule := NewFeatureRule([]FeatureFlag{"tool"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("tool") + }) + resourceRule := NewFeatureRule([]FeatureFlag{"resource"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("resource") + }) + promptRule := NewFeatureRule([]FeatureFlag{"prompt"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("prompt") + }) + tools := []ServerTool{mockTool("tool", "toolset1", true)} + tools[0].FeatureRule = toolRule + tools[0].Tool.Meta = map[string]any{"ui": "resource"} + resources := []ServerResourceTemplate{mockResource("resource", "toolset1", "uri")} + resources[0].FeatureRule = resourceRule + resources[0].Template.Meta = map[string]any{"resource": true} + prompts := []ServerPrompt{mockPrompt("prompt", "toolset1")} + prompts[0].FeatureRule = promptRule + prompts[0].Prompt.Meta = map[string]any{"prompt": true} + + checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + return flag == "tool" || flag == "resource" || flag == "prompt" || flag == mcpAppsFeatureFlag, nil + } + inv := mustBuild(t, NewBuilder(). + SetTools(tools). + SetResources(resources). + SetPrompts(prompts). + WithToolsets([]string{"all"}). + WithFeatureChecker(checker)) + + tools[0].FeatureRule.features[0] = "changed" + delete(tools[0].Tool.Meta, "ui") + resources[0].FeatureRule.features[0] = "changed" + delete(resources[0].Template.Meta, "resource") + prompts[0].FeatureRule.features[0] = "changed" + delete(prompts[0].Prompt.Meta, "prompt") + + require.Equal(t, []FeatureFlag{"prompt", "remote_mcp_ui_apps", "resource", "tool"}, inv.RequiredFeatures()) + require.Len(t, inv.AvailableTools(context.Background()), 1) + require.Len(t, inv.AvailableResourceTemplates(context.Background()), 1) + require.Len(t, inv.AvailablePrompts(context.Background()), 1) + require.Contains(t, inv.tools[0].Tool.Meta, "ui") + require.Contains(t, inv.resourceTemplates[0].Template.Meta, "resource") + require.Contains(t, inv.prompts[0].Prompt.Meta, "prompt") + + availableTools := inv.AvailableTools(context.Background()) + availableResources := inv.AvailableResourceTemplates(context.Background()) + availablePrompts := inv.AvailablePrompts(context.Background()) + delete(availableTools[0].Tool.Meta, "ui") + availableTools[0].FeatureRule.features[0] = "changed" + delete(availableResources[0].Template.Meta, "resource") + availableResources[0].FeatureRule.features[0] = "changed" + delete(availablePrompts[0].Prompt.Meta, "prompt") + availablePrompts[0].FeatureRule.features[0] = "changed" + + require.Contains(t, inv.AvailableTools(context.Background())[0].Tool.Meta, "ui") + require.Contains(t, inv.AvailableResourceTemplates(context.Background())[0].Template.Meta, "resource") + require.Contains(t, inv.AvailablePrompts(context.Background())[0].Prompt.Meta, "prompt") +} + func TestServerToolHasHandler(t *testing.T) { // Tool with handler toolWithHandler := mockTool("has_handler", "toolset1", true) From 4ea15554facedfb0b7fbd89b13b5521cf8116c15 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 1 Sep 2026 16:56:19 +0200 Subject: [PATCH 06/15] fix(features): share registration resolution Seed feature state once for direct registration so tools, resources, and prompts use one consistent snapshot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/registry.go | 1 + pkg/inventory/registry_test.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 5dc2b081e4..add2f49256 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -332,6 +332,7 @@ func (r *Inventory) RegisterPrompts(ctx context.Context, s *mcp.Server) { // RegisterAll registers all available tools, resources, and prompts with the server. // The context is used for feature flag evaluation. func (r *Inventory) RegisterAll(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { + ctx = r.WithResolvedFeatures(ctx) r.RegisterTools(ctx, s, deps, middleware...) r.RegisterResourceTemplates(ctx, s, deps) r.RegisterPrompts(ctx, s) diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 5879808b2c..5a9ec1e8bf 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1221,6 +1221,53 @@ func TestFeatureFlagPrompts(t *testing.T) { } } +func TestRegisterAllSharesFeatureResolution(t *testing.T) { + const feature = FeatureFlag("shared") + rule := NewFeatureRule([]FeatureFlag{feature}, func(featureAsBool FeatureResolver) bool { + return featureAsBool(feature) + }) + tool := mockTool("tool", "toolset1", true) + tool.FeatureRule = rule + resource := mockResource("resource", "toolset1", "test://{id}") + resource.FeatureRule = rule + prompt := mockPrompt("prompt", "toolset1") + prompt.FeatureRule = rule + + calls := 0 + checker := func(context.Context, FeatureFlag) (bool, error) { + calls++ + return calls == 1, nil + } + inv := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{tool}). + SetResources([]ServerResourceTemplate{resource}). + SetPrompts([]ServerPrompt{prompt}). + WithToolsets([]string{"all"}). + WithFeatureChecker(checker)) + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + inv.RegisterAll(context.Background(), server, nil) + clientTransport, serverTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(context.Background(), serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) + clientSession, err := client.Connect(context.Background(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + tools, err := clientSession.ListTools(context.Background(), nil) + require.NoError(t, err) + require.Len(t, tools.Tools, 1) + resources, err := clientSession.ListResourceTemplates(context.Background(), nil) + require.NoError(t, err) + require.Len(t, resources.ResourceTemplates, 1) + prompts, err := clientSession.ListPrompts(context.Background(), nil) + require.NoError(t, err) + require.Len(t, prompts.Prompts, 1) + require.Equal(t, 1, calls) +} + func TestFeatureMetadataIsCachedAndNarrowed(t *testing.T) { tools := []ServerTool{ mockToolWithFlags("tool_x", "toolset1", true, "x", ""), From 7921f78b8e36f159fb6889f4c482ed1c1be2fe5b Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 11:41:27 +0200 Subject: [PATCH 07/15] fix(features): isolate metadata and validate variants Deep-clone JSON metadata at inventory boundaries and exhaustively reject duplicate tool variants that can be enabled together. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/github/tools_validation_test.go | 68 ++++++++++++++++----- pkg/inventory/builder.go | 6 +- pkg/inventory/metadata.go | 41 +++++++++++++ pkg/inventory/registry_test.go | 92 ++++++++++++++++++++++------- 4 files changed, 167 insertions(+), 40 deletions(-) create mode 100644 pkg/inventory/metadata.go diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index 31b39d1a45..5029bee2c9 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -166,34 +166,70 @@ func TestToolReadOnlyHintConsistency(t *testing.T) { } } -// TestNoDuplicateToolNames ensures all tools have unique names +// TestNoDuplicateToolNames ensures duplicate names cannot be enabled together. func TestNoDuplicateToolNames(t *testing.T) { tools := AllTools(stubTranslation) - seen := make(map[string]bool) - featureFlagged := make(map[string]bool) + rulesByName := make(map[string][]inventory.FeatureRule) + for _, tool := range tools { + rulesByName[tool.Tool.Name] = append(rulesByName[tool.Tool.Name], tool.FeatureRule) + } // get_label is intentionally in both issues and labels toolsets for conformance // with original behavior where it was registered in both - allowedDuplicates := map[string]bool{ - "get_label": true, + for name, rules := range rulesByName { + if name == "get_label" || len(rules) < 2 { + continue + } + assert.False(t, featureRulesOverlap(rules), "tool variants for %q can be enabled together", name) } +} - // First pass: identify tools that have feature flags (mutually exclusive at runtime) - for _, tool := range tools { - if !tool.FeatureRule.IsZero() { - featureFlagged[tool.Tool.Name] = true +func featureRulesOverlap(rules []inventory.FeatureRule) bool { + positions := make(map[inventory.FeatureFlag]uint) + for _, rule := range rules { + for _, feature := range rule.Features() { + if _, ok := positions[feature]; !ok { + positions[feature] = uint(len(positions)) + } } } + if len(positions) > 16 { + return true + } - for _, tool := range tools { - name := tool.Tool.Name - // Allow duplicates for explicitly allowed tools and feature-flagged tools - if !allowedDuplicates[name] && !featureFlagged[name] { - assert.False(t, seen[name], - "Duplicate tool name found: %q", name) + for assignment := range 1 << len(positions) { + enabled := 0 + for _, rule := range rules { + if rule.IsZero() || rule.Enabled(func(feature inventory.FeatureFlag) bool { + return assignment&(1< 1 { + return true } - seen[name] = true } + return false +} + +func TestFeatureRulesOverlap(t *testing.T) { + flag := inventory.FeatureFlag("flag") + enabled := inventory.NewFeatureRule([]inventory.FeatureFlag{flag}, func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(flag) + }) + disabled := inventory.NewFeatureRule([]inventory.FeatureFlag{flag}, func(featureAsBool inventory.FeatureResolver) bool { + return !featureAsBool(flag) + }) + otherFlag := inventory.FeatureFlag("other") + otherEnabled := inventory.NewFeatureRule([]inventory.FeatureFlag{otherFlag}, func(featureAsBool inventory.FeatureResolver) bool { + return featureAsBool(otherFlag) + }) + + assert.True(t, featureRulesOverlap([]inventory.FeatureRule{enabled, enabled})) + assert.True(t, featureRulesOverlap([]inventory.FeatureRule{enabled, otherEnabled})) + assert.True(t, featureRulesOverlap([]inventory.FeatureRule{{}, enabled})) + assert.False(t, featureRulesOverlap([]inventory.FeatureRule{enabled, disabled})) } func TestMCPAppsFeatureFlagMatchesInventory(t *testing.T) { diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index f75013affe..1ecf203224 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -93,19 +93,19 @@ func (b *Builder) SetPrompts(prompts []ServerPrompt) *Builder { } func cloneServerTool(tool ServerTool) ServerTool { - tool.Tool.Meta = maps.Clone(tool.Tool.Meta) + tool.Tool.Meta = cloneMeta(tool.Tool.Meta) tool.FeatureRule = tool.FeatureRule.clone() return tool } func cloneResourceTemplate(resource ServerResourceTemplate) ServerResourceTemplate { - resource.Template.Meta = maps.Clone(resource.Template.Meta) + resource.Template.Meta = cloneMeta(resource.Template.Meta) resource.FeatureRule = resource.FeatureRule.clone() return resource } func clonePrompt(prompt ServerPrompt) ServerPrompt { - prompt.Prompt.Meta = maps.Clone(prompt.Prompt.Meta) + prompt.Prompt.Meta = cloneMeta(prompt.Prompt.Meta) prompt.FeatureRule = prompt.FeatureRule.clone() return prompt } diff --git a/pkg/inventory/metadata.go b/pkg/inventory/metadata.go new file mode 100644 index 0000000000..75eaa9aa15 --- /dev/null +++ b/pkg/inventory/metadata.go @@ -0,0 +1,41 @@ +package inventory + +import ( + "slices" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +func cloneMeta(meta mcp.Meta) mcp.Meta { + if meta == nil { + return nil + } + clone := make(mcp.Meta, len(meta)) + for key, value := range meta { + clone[key] = cloneMetaValue(value) + } + return clone +} + +func cloneMetaValue(value any) any { + switch value := value.(type) { + case mcp.Meta: + return cloneMeta(value) + case map[string]any: + clone := make(map[string]any, len(value)) + for key, item := range value { + clone[key] = cloneMetaValue(item) + } + return clone + case []any: + clone := make([]any, len(value)) + for i, item := range value { + clone[i] = cloneMetaValue(item) + } + return clone + case []string: + return slices.Clone(value) + default: + return value + } +} diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 5a9ec1e8bf..1f73295a42 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1302,52 +1302,102 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { }) tools := []ServerTool{mockTool("tool", "toolset1", true)} tools[0].FeatureRule = toolRule - tools[0].Tool.Meta = map[string]any{"ui": "resource"} + tools[0].Tool.Meta = nestedTestMeta("ui", "tool") resources := []ServerResourceTemplate{mockResource("resource", "toolset1", "uri")} resources[0].FeatureRule = resourceRule - resources[0].Template.Meta = map[string]any{"resource": true} + resources[0].Template.Meta = nestedTestMeta("resource", "resource") prompts := []ServerPrompt{mockPrompt("prompt", "toolset1")} prompts[0].FeatureRule = promptRule - prompts[0].Prompt.Meta = map[string]any{"prompt": true} + prompts[0].Prompt.Meta = nestedTestMeta("prompt", "prompt") checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "tool" || flag == "resource" || flag == "prompt" || flag == mcpAppsFeatureFlag, nil } - inv := mustBuild(t, NewBuilder(). + builder := NewBuilder(). SetTools(tools). SetResources(resources). SetPrompts(prompts). WithToolsets([]string{"all"}). - WithFeatureChecker(checker)) + WithFeatureChecker(checker) + setNestedTestMeta(tools[0].Tool.Meta, "ui", "after-set") + setNestedTestMeta(resources[0].Template.Meta, "resource", "after-set") + setNestedTestMeta(prompts[0].Prompt.Meta, "prompt", "after-set") + inv := mustBuild(t, builder) tools[0].FeatureRule.features[0] = "changed" - delete(tools[0].Tool.Meta, "ui") + setNestedTestMeta(tools[0].Tool.Meta, "ui", "after-build") resources[0].FeatureRule.features[0] = "changed" - delete(resources[0].Template.Meta, "resource") + setNestedTestMeta(resources[0].Template.Meta, "resource", "after-build") prompts[0].FeatureRule.features[0] = "changed" - delete(prompts[0].Prompt.Meta, "prompt") + setNestedTestMeta(prompts[0].Prompt.Meta, "prompt", "after-build") require.Equal(t, []FeatureFlag{"prompt", "remote_mcp_ui_apps", "resource", "tool"}, inv.RequiredFeatures()) - require.Len(t, inv.AvailableTools(context.Background()), 1) - require.Len(t, inv.AvailableResourceTemplates(context.Background()), 1) - require.Len(t, inv.AvailablePrompts(context.Background()), 1) - require.Contains(t, inv.tools[0].Tool.Meta, "ui") - require.Contains(t, inv.resourceTemplates[0].Template.Meta, "resource") - require.Contains(t, inv.prompts[0].Prompt.Meta, "prompt") - availableTools := inv.AvailableTools(context.Background()) availableResources := inv.AvailableResourceTemplates(context.Background()) availablePrompts := inv.AvailablePrompts(context.Background()) - delete(availableTools[0].Tool.Meta, "ui") + require.Len(t, availableTools, 1) + require.Len(t, availableResources, 1) + require.Len(t, availablePrompts, 1) + requireNestedTestMeta(t, availableTools[0].Tool.Meta, "ui", "tool") + requireNestedTestMeta(t, availableResources[0].Template.Meta, "resource", "resource") + requireNestedTestMeta(t, availablePrompts[0].Prompt.Meta, "prompt", "prompt") + + setNestedTestMeta(availableTools[0].Tool.Meta, "ui", "after-available") availableTools[0].FeatureRule.features[0] = "changed" - delete(availableResources[0].Template.Meta, "resource") + setNestedTestMeta(availableResources[0].Template.Meta, "resource", "after-available") availableResources[0].FeatureRule.features[0] = "changed" - delete(availablePrompts[0].Prompt.Meta, "prompt") + setNestedTestMeta(availablePrompts[0].Prompt.Meta, "prompt", "after-available") availablePrompts[0].FeatureRule.features[0] = "changed" - require.Contains(t, inv.AvailableTools(context.Background())[0].Tool.Meta, "ui") - require.Contains(t, inv.AvailableResourceTemplates(context.Background())[0].Template.Meta, "resource") - require.Contains(t, inv.AvailablePrompts(context.Background())[0].Prompt.Meta, "prompt") + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + inv.RegisterAll(context.Background(), server, nil) + clientTransport, serverTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(context.Background(), serverTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) + clientSession, err := client.Connect(context.Background(), clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + registeredTools, err := clientSession.ListTools(context.Background(), nil) + require.NoError(t, err) + requireNestedTestMeta(t, registeredTools.Tools[0].Meta, "ui", "tool") + registeredResources, err := clientSession.ListResourceTemplates(context.Background(), nil) + require.NoError(t, err) + requireNestedTestMeta(t, registeredResources.ResourceTemplates[0].Meta, "resource", "resource") + registeredPrompts, err := clientSession.ListPrompts(context.Background(), nil) + require.NoError(t, err) + requireNestedTestMeta(t, registeredPrompts.Prompts[0].Meta, "prompt", "prompt") +} + +func nestedTestMeta(key, value string) mcp.Meta { + return mcp.Meta{ + key: map[string]any{ + "objects": []any{map[string]any{"value": value}}, + "strings": []string{value}, + }, + } +} + +func setNestedTestMeta(meta mcp.Meta, key, value string) { + nested := meta[key].(map[string]any) + nested["objects"].([]any)[0].(map[string]any)["value"] = value + nested["strings"].([]string)[0] = value +} + +func requireNestedTestMeta(t *testing.T, meta mcp.Meta, key, value string) { + t.Helper() + nested := meta[key].(map[string]any) + require.Equal(t, value, nested["objects"].([]any)[0].(map[string]any)["value"]) + switch strings := nested["strings"].(type) { + case []string: + require.Equal(t, value, strings[0]) + case []any: + require.Equal(t, value, strings[0]) + default: + require.Failf(t, "unexpected strings metadata", "type %T", strings) + } } func TestServerToolHasHandler(t *testing.T) { From e470871f274304cbc700afca863500a1f759a9f2 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 11:49:48 +0200 Subject: [PATCH 08/15] fix(inventory): isolate tool lookup results Return owned tool metadata from unfiltered lookup APIs so callers cannot mutate cached inventory state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/registry.go | 8 ++++++-- pkg/inventory/registry_test.go | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index add2f49256..3a79fe76f1 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -364,7 +364,8 @@ func (r *Inventory) ResolveToolAliases(toolNames []string) (resolved []string, a func (r *Inventory) FindToolByName(toolName string) (*ServerTool, ToolsetID, error) { for i := range r.tools { if r.tools[i].Tool.Name == toolName { - return &r.tools[i], r.tools[i].Toolset.ID, nil + tool := cloneServerTool(r.tools[i]) + return &tool, tool.Toolset.ID, nil } } return nil, "", NewToolDoesNotExistError(toolName) @@ -377,7 +378,10 @@ func (r *Inventory) HasToolset(toolsetID ToolsetID) bool { // AllTools returns all tools without any filtering, sorted deterministically. func (r *Inventory) AllTools() []ServerTool { - result := slices.Clone(r.tools) + result := make([]ServerTool, len(r.tools)) + for i := range r.tools { + result[i] = cloneServerTool(r.tools[i]) + } // Sort deterministically: by toolset ID, then by tool name sort.Slice(result, func(i, j int) bool { diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 1f73295a42..63c86fc417 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1349,6 +1349,12 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { setNestedTestMeta(availablePrompts[0].Prompt.Meta, "prompt", "after-available") availablePrompts[0].FeatureRule.features[0] = "changed" + allTools := inv.AllTools() + setNestedTestMeta(allTools[0].Tool.Meta, "ui", "after-all") + foundTool, _, err := inv.FindToolByName("tool") + require.NoError(t, err) + setNestedTestMeta(foundTool.Tool.Meta, "ui", "after-find") + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) inv.RegisterAll(context.Background(), server, nil) clientTransport, serverTransport := mcp.NewInMemoryTransports() From e070a779d16b0749692054a591dde2f1f4cdcd61 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 12:17:58 +0200 Subject: [PATCH 09/15] fix(inventory): normalize owned metadata Canonicalize JSON metadata at builder boundaries, report invalid values from Build, and preserve wire semantics for typed mutable values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/builder.go | 53 +++++++++++++-- pkg/inventory/metadata.go | 24 +++++-- pkg/inventory/registry_test.go | 116 +++++++++++++++++++++++++++------ 3 files changed, 163 insertions(+), 30 deletions(-) diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 1ecf203224..55d9a0eb8a 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -24,8 +24,8 @@ const mcpAppsFeatureFlag FeatureFlag = "remote_mcp_ui_apps" type ToolFilter func(ctx context.Context, tool *ServerTool) (bool, error) // Builder builds a Registry with the specified configuration. SetTools, -// SetResources, and SetPrompts copy the feature and metadata state retained by -// the inventory. +// SetResources, and SetPrompts normalize retained metadata through JSON so the +// inventory owns it; Build reports metadata that cannot be represented as JSON. // Use NewBuilder to create a builder, chain configuration methods, // then call Build() to create the final inventory. // @@ -45,6 +45,9 @@ type Builder struct { tools []ServerTool resourceTemplates []ServerResourceTemplate prompts []ServerPrompt + toolsErr error + resourcesErr error + promptsErr error deprecatedAliases map[string]string // Configuration options (processed at Build time) @@ -68,8 +71,13 @@ func NewBuilder() *Builder { // SetTools sets the tools for the inventory. Returns self for chaining. func (b *Builder) SetTools(tools []ServerTool) *Builder { b.tools = slices.Clone(tools) + b.toolsErr = nil for i := range b.tools { - b.tools[i] = cloneServerTool(b.tools[i]) + tool, err := ownServerTool(b.tools[i]) + if err != nil { + b.toolsErr = errors.Join(b.toolsErr, fmt.Errorf("tool %q metadata: %w", b.tools[i].Tool.Name, err)) + } + b.tools[i] = tool } return b } @@ -77,8 +85,13 @@ func (b *Builder) SetTools(tools []ServerTool) *Builder { // SetResources sets the resource templates for the inventory. Returns self for chaining. func (b *Builder) SetResources(resources []ServerResourceTemplate) *Builder { b.resourceTemplates = slices.Clone(resources) + b.resourcesErr = nil for i := range b.resourceTemplates { - b.resourceTemplates[i] = cloneResourceTemplate(b.resourceTemplates[i]) + resource, err := ownResourceTemplate(b.resourceTemplates[i]) + if err != nil { + b.resourcesErr = errors.Join(b.resourcesErr, fmt.Errorf("resource template %q metadata: %w", b.resourceTemplates[i].Template.Name, err)) + } + b.resourceTemplates[i] = resource } return b } @@ -86,12 +99,38 @@ func (b *Builder) SetResources(resources []ServerResourceTemplate) *Builder { // SetPrompts sets the prompts for the inventory. Returns self for chaining. func (b *Builder) SetPrompts(prompts []ServerPrompt) *Builder { b.prompts = slices.Clone(prompts) + b.promptsErr = nil for i := range b.prompts { - b.prompts[i] = clonePrompt(b.prompts[i]) + prompt, err := ownPrompt(b.prompts[i]) + if err != nil { + b.promptsErr = errors.Join(b.promptsErr, fmt.Errorf("prompt %q metadata: %w", b.prompts[i].Prompt.Name, err)) + } + b.prompts[i] = prompt } return b } +func ownServerTool(tool ServerTool) (ServerTool, error) { + meta, err := normalizeMeta(tool.Tool.Meta) + tool.Tool.Meta = meta + tool.FeatureRule = tool.FeatureRule.clone() + return tool, err +} + +func ownResourceTemplate(resource ServerResourceTemplate) (ServerResourceTemplate, error) { + meta, err := normalizeMeta(resource.Template.Meta) + resource.Template.Meta = meta + resource.FeatureRule = resource.FeatureRule.clone() + return resource, err +} + +func ownPrompt(prompt ServerPrompt) (ServerPrompt, error) { + meta, err := normalizeMeta(prompt.Prompt.Meta) + prompt.Prompt.Meta = meta + prompt.FeatureRule = prompt.FeatureRule.clone() + return prompt, err +} + func cloneServerTool(tool ServerTool) ServerTool { tool.Tool.Meta = cloneMeta(tool.Tool.Meta) tool.FeatureRule = tool.FeatureRule.clone() @@ -234,6 +273,10 @@ func cleanTools(tools []string) []string { // (i.e., they don't exist in the tool set and are not deprecated aliases). // This ensures invalid tool configurations fail fast at build time. func (b *Builder) Build() (*Inventory, error) { + if err := errors.Join(b.toolsErr, b.resourcesErr, b.promptsErr); err != nil { + return nil, fmt.Errorf("invalid inventory metadata: %w", err) + } + tools := b.tools filters := b.filters diff --git a/pkg/inventory/metadata.go b/pkg/inventory/metadata.go index 75eaa9aa15..113360026b 100644 --- a/pkg/inventory/metadata.go +++ b/pkg/inventory/metadata.go @@ -1,11 +1,27 @@ package inventory import ( - "slices" + "encoding/json" + "fmt" "github.com/modelcontextprotocol/go-sdk/mcp" ) +func normalizeMeta(meta mcp.Meta) (mcp.Meta, error) { + if meta == nil { + return nil, nil + } + data, err := json.Marshal(meta) + if err != nil { + return nil, fmt.Errorf("marshal metadata: %w", err) + } + var normalized mcp.Meta + if err := json.Unmarshal(data, &normalized); err != nil { + return nil, fmt.Errorf("unmarshal metadata: %w", err) + } + return normalized, nil +} + func cloneMeta(meta mcp.Meta) mcp.Meta { if meta == nil { return nil @@ -33,9 +49,9 @@ func cloneMetaValue(value any) any { clone[i] = cloneMetaValue(item) } return clone - case []string: - return slices.Clone(value) - default: + case nil, bool, float64, string: return value + default: + panic(fmt.Sprintf("metadata value %T is not normalized", value)) } } diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 63c86fc417..c7e25c9105 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -2,6 +2,7 @@ package inventory import ( "context" + "encoding/base64" "encoding/json" "fmt" "testing" @@ -1309,6 +1310,9 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { prompts := []ServerPrompt{mockPrompt("prompt", "toolset1")} prompts[0].FeatureRule = promptRule prompts[0].Prompt.Meta = nestedTestMeta("prompt", "prompt") + expectedToolMeta := mustMarshalJSON(t, tools[0].Tool.Meta) + expectedResourceMeta := mustMarshalJSON(t, resources[0].Template.Meta) + expectedPromptMeta := mustMarshalJSON(t, prompts[0].Prompt.Meta) checker := func(_ context.Context, flag FeatureFlag) (bool, error) { return flag == "tool" || flag == "resource" || flag == "prompt" || flag == mcpAppsFeatureFlag, nil @@ -1320,16 +1324,16 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { WithToolsets([]string{"all"}). WithFeatureChecker(checker) - setNestedTestMeta(tools[0].Tool.Meta, "ui", "after-set") - setNestedTestMeta(resources[0].Template.Meta, "resource", "after-set") - setNestedTestMeta(prompts[0].Prompt.Meta, "prompt", "after-set") + mutateSourceTestMeta(tools[0].Tool.Meta, "ui", "after-set", 's') + mutateSourceTestMeta(resources[0].Template.Meta, "resource", "after-set", 's') + mutateSourceTestMeta(prompts[0].Prompt.Meta, "prompt", "after-set", 's') inv := mustBuild(t, builder) tools[0].FeatureRule.features[0] = "changed" - setNestedTestMeta(tools[0].Tool.Meta, "ui", "after-build") + mutateSourceTestMeta(tools[0].Tool.Meta, "ui", "after-build", 'b') resources[0].FeatureRule.features[0] = "changed" - setNestedTestMeta(resources[0].Template.Meta, "resource", "after-build") + mutateSourceTestMeta(resources[0].Template.Meta, "resource", "after-build", 'b') prompts[0].FeatureRule.features[0] = "changed" - setNestedTestMeta(prompts[0].Prompt.Meta, "prompt", "after-build") + mutateSourceTestMeta(prompts[0].Prompt.Meta, "prompt", "after-build", 'b') require.Equal(t, []FeatureFlag{"prompt", "remote_mcp_ui_apps", "resource", "tool"}, inv.RequiredFeatures()) availableTools := inv.AvailableTools(context.Background()) @@ -1342,18 +1346,18 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { requireNestedTestMeta(t, availableResources[0].Template.Meta, "resource", "resource") requireNestedTestMeta(t, availablePrompts[0].Prompt.Meta, "prompt", "prompt") - setNestedTestMeta(availableTools[0].Tool.Meta, "ui", "after-available") + mutateNormalizedTestMeta(availableTools[0].Tool.Meta, "ui", "after-available") availableTools[0].FeatureRule.features[0] = "changed" - setNestedTestMeta(availableResources[0].Template.Meta, "resource", "after-available") + mutateNormalizedTestMeta(availableResources[0].Template.Meta, "resource", "after-available") availableResources[0].FeatureRule.features[0] = "changed" - setNestedTestMeta(availablePrompts[0].Prompt.Meta, "prompt", "after-available") + mutateNormalizedTestMeta(availablePrompts[0].Prompt.Meta, "prompt", "after-available") availablePrompts[0].FeatureRule.features[0] = "changed" allTools := inv.AllTools() - setNestedTestMeta(allTools[0].Tool.Meta, "ui", "after-all") + mutateNormalizedTestMeta(allTools[0].Tool.Meta, "ui", "after-all") foundTool, _, err := inv.FindToolByName("tool") require.NoError(t, err) - setNestedTestMeta(foundTool.Tool.Meta, "ui", "after-find") + mutateNormalizedTestMeta(foundTool.Tool.Meta, "ui", "after-find") server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) inv.RegisterAll(context.Background(), server, nil) @@ -1369,40 +1373,110 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { registeredTools, err := clientSession.ListTools(context.Background(), nil) require.NoError(t, err) requireNestedTestMeta(t, registeredTools.Tools[0].Meta, "ui", "tool") + require.JSONEq(t, string(expectedToolMeta), string(mustMarshalJSON(t, registeredTools.Tools[0].Meta))) registeredResources, err := clientSession.ListResourceTemplates(context.Background(), nil) require.NoError(t, err) requireNestedTestMeta(t, registeredResources.ResourceTemplates[0].Meta, "resource", "resource") + require.JSONEq(t, string(expectedResourceMeta), string(mustMarshalJSON(t, registeredResources.ResourceTemplates[0].Meta))) registeredPrompts, err := clientSession.ListPrompts(context.Background(), nil) require.NoError(t, err) requireNestedTestMeta(t, registeredPrompts.Prompts[0].Meta, "prompt", "prompt") + require.JSONEq(t, string(expectedPromptMeta), string(mustMarshalJSON(t, registeredPrompts.Prompts[0].Meta))) +} + +type testMetaMap map[string]string +type testMetaSlice []string +type testMetaPointer struct { + Value string `json:"value"` } func nestedTestMeta(key, value string) mcp.Meta { return mcp.Meta{ key: map[string]any{ - "objects": []any{map[string]any{"value": value}}, - "strings": []string{value}, + "objects": []any{map[string]any{"value": value}}, + "strings": []string{value}, + "typed_map": testMetaMap{"value": value}, + "typed_slice": testMetaSlice{value}, + "bytes": []byte(value), + "pointer": &testMetaPointer{Value: value}, }, } } -func setNestedTestMeta(meta mcp.Meta, key, value string) { +func mutateSourceTestMeta(meta mcp.Meta, key, value string, marker byte) { nested := meta[key].(map[string]any) nested["objects"].([]any)[0].(map[string]any)["value"] = value nested["strings"].([]string)[0] = value + nested["typed_map"].(testMetaMap)["value"] = value + nested["typed_slice"].(testMetaSlice)[0] = value + nested["bytes"].([]byte)[0] = marker + nested["pointer"].(*testMetaPointer).Value = value +} + +func mutateNormalizedTestMeta(meta mcp.Meta, key, value string) { + nested := meta[key].(map[string]any) + nested["objects"].([]any)[0].(map[string]any)["value"] = value + nested["strings"].([]any)[0] = value + nested["typed_map"].(map[string]any)["value"] = value + nested["typed_slice"].([]any)[0] = value + nested["bytes"] = value + nested["pointer"].(map[string]any)["value"] = value } func requireNestedTestMeta(t *testing.T, meta mcp.Meta, key, value string) { t.Helper() nested := meta[key].(map[string]any) require.Equal(t, value, nested["objects"].([]any)[0].(map[string]any)["value"]) - switch strings := nested["strings"].(type) { - case []string: - require.Equal(t, value, strings[0]) - case []any: - require.Equal(t, value, strings[0]) - default: - require.Failf(t, "unexpected strings metadata", "type %T", strings) + require.Equal(t, value, nested["strings"].([]any)[0]) + require.Equal(t, value, nested["typed_map"].(map[string]any)["value"]) + require.Equal(t, value, nested["typed_slice"].([]any)[0]) + require.Equal(t, base64.StdEncoding.EncodeToString([]byte(value)), nested["bytes"]) + require.Equal(t, value, nested["pointer"].(map[string]any)["value"]) +} + +func mustMarshalJSON(t *testing.T, value any) []byte { + t.Helper() + data, err := json.Marshal(value) + require.NoError(t, err) + return data +} + +func TestBuildRejectsInvalidMetadata(t *testing.T) { + tests := []struct { + name string + builder *Builder + errorText string + }{ + { + name: "tool", + builder: NewBuilder().SetTools([]ServerTool{{ + Tool: mcp.Tool{Name: "tool", Meta: mcp.Meta{"invalid": make(chan int)}}, + }}), + errorText: `tool "tool" metadata`, + }, + { + name: "resource", + builder: NewBuilder().SetResources([]ServerResourceTemplate{{ + Template: mcp.ResourceTemplate{Name: "resource", Meta: mcp.Meta{"invalid": make(chan int)}}, + }}), + errorText: `resource template "resource" metadata`, + }, + { + name: "prompt", + builder: NewBuilder().SetPrompts([]ServerPrompt{{ + Prompt: mcp.Prompt{Name: "prompt", Meta: mcp.Meta{"invalid": make(chan int)}}, + }}), + errorText: `prompt "prompt" metadata`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := tt.builder.Build() + require.ErrorContains(t, err, "invalid inventory metadata") + require.ErrorContains(t, err, tt.errorText) + require.ErrorContains(t, err, "unsupported type: chan int") + }) } } From f479917f81a0544f62f543a6ef935cd9d4bf779d Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 13:50:08 +0200 Subject: [PATCH 10/15] fix(inventory): preserve metadata number precision Decode normalized metadata with UseNumber so large integers and other JSON number forms retain their exact wire representation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/metadata.go | 7 +++- pkg/inventory/registry_test.go | 70 ++++++++++++++++++++++++++++++++-- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/pkg/inventory/metadata.go b/pkg/inventory/metadata.go index 113360026b..11728d8994 100644 --- a/pkg/inventory/metadata.go +++ b/pkg/inventory/metadata.go @@ -1,6 +1,7 @@ package inventory import ( + "bytes" "encoding/json" "fmt" @@ -16,7 +17,9 @@ func normalizeMeta(meta mcp.Meta) (mcp.Meta, error) { return nil, fmt.Errorf("marshal metadata: %w", err) } var normalized mcp.Meta - if err := json.Unmarshal(data, &normalized); err != nil { + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + if err := decoder.Decode(&normalized); err != nil { return nil, fmt.Errorf("unmarshal metadata: %w", err) } return normalized, nil @@ -49,7 +52,7 @@ func cloneMetaValue(value any) any { clone[i] = cloneMetaValue(item) } return clone - case nil, bool, float64, string: + case nil, bool, string, json.Number: return value default: panic(fmt.Sprintf("metadata value %T is not normalized", value)) diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index c7e25c9105..ba0f921070 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1,6 +1,7 @@ package inventory import ( + "bytes" "context" "encoding/base64" "encoding/json" @@ -1345,6 +1346,12 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { requireNestedTestMeta(t, availableTools[0].Tool.Meta, "ui", "tool") requireNestedTestMeta(t, availableResources[0].Template.Meta, "resource", "resource") requireNestedTestMeta(t, availablePrompts[0].Prompt.Meta, "prompt", "prompt") + requireExactTestMetaNumbers(t, availableTools[0].Tool.Meta, "ui") + requireExactTestMetaNumbers(t, availableResources[0].Template.Meta, "resource") + requireExactTestMetaNumbers(t, availablePrompts[0].Prompt.Meta, "prompt") + requireJSONEquivalent(t, expectedToolMeta, mustMarshalJSON(t, availableTools[0].Tool.Meta)) + requireJSONEquivalent(t, expectedResourceMeta, mustMarshalJSON(t, availableResources[0].Template.Meta)) + requireJSONEquivalent(t, expectedPromptMeta, mustMarshalJSON(t, availablePrompts[0].Prompt.Meta)) mutateNormalizedTestMeta(availableTools[0].Tool.Meta, "ui", "after-available") availableTools[0].FeatureRule.features[0] = "changed" @@ -1387,9 +1394,17 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { type testMetaMap map[string]string type testMetaSlice []string type testMetaPointer struct { - Value string `json:"value"` + Value string `json:"value"` + Decimal float64 `json:"decimal"` } +const ( + testLargeSigned int64 = 9007199254740993 + testLargeUnsigned uint64 = 18446744073709551614 + testExponent = 6.022e23 + testDecimal = 1.2345678901234567 +) + func nestedTestMeta(key, value string) mcp.Meta { return mcp.Meta{ key: map[string]any{ @@ -1398,11 +1413,24 @@ func nestedTestMeta(key, value string) mcp.Meta { "typed_map": testMetaMap{"value": value}, "typed_slice": testMetaSlice{value}, "bytes": []byte(value), - "pointer": &testMetaPointer{Value: value}, + "pointer": &testMetaPointer{Value: value, Decimal: testDecimal}, + "numbers": &testMetaNumbers{ + Signed: testLargeSigned, + Unsigned: testLargeUnsigned, + Exponent: testExponent, + Decimal: testDecimal, + }, }, } } +type testMetaNumbers struct { + Signed int64 `json:"signed"` + Unsigned uint64 `json:"unsigned"` + Exponent float64 `json:"exponent"` + Decimal float64 `json:"decimal"` +} + func mutateSourceTestMeta(meta mcp.Meta, key, value string, marker byte) { nested := meta[key].(map[string]any) nested["objects"].([]any)[0].(map[string]any)["value"] = value @@ -1411,6 +1439,7 @@ func mutateSourceTestMeta(meta mcp.Meta, key, value string, marker byte) { nested["typed_slice"].(testMetaSlice)[0] = value nested["bytes"].([]byte)[0] = marker nested["pointer"].(*testMetaPointer).Value = value + nested["numbers"].(*testMetaNumbers).Signed = 1 } func mutateNormalizedTestMeta(meta mcp.Meta, key, value string) { @@ -1421,6 +1450,7 @@ func mutateNormalizedTestMeta(meta mcp.Meta, key, value string) { nested["typed_slice"].([]any)[0] = value nested["bytes"] = value nested["pointer"].(map[string]any)["value"] = value + nested["numbers"].(map[string]any)["signed"] = json.Number("1") } func requireNestedTestMeta(t *testing.T, meta mcp.Meta, key, value string) { @@ -1434,6 +1464,17 @@ func requireNestedTestMeta(t *testing.T, meta mcp.Meta, key, value string) { require.Equal(t, value, nested["pointer"].(map[string]any)["value"]) } +func requireExactTestMetaNumbers(t *testing.T, meta mcp.Meta, key string) { + t.Helper() + nested := meta[key].(map[string]any) + require.Equal(t, json.Number("1.2345678901234567"), nested["pointer"].(map[string]any)["decimal"]) + numbers := nested["numbers"].(map[string]any) + require.Equal(t, json.Number("9007199254740993"), numbers["signed"]) + require.Equal(t, json.Number("18446744073709551614"), numbers["unsigned"]) + require.Equal(t, json.Number("6.022e+23"), numbers["exponent"]) + require.Equal(t, json.Number("1.2345678901234567"), numbers["decimal"]) +} + func mustMarshalJSON(t *testing.T, value any) []byte { t.Helper() data, err := json.Marshal(value) @@ -1441,6 +1482,18 @@ func mustMarshalJSON(t *testing.T, value any) []byte { return data } +func requireJSONEquivalent(t *testing.T, expected, actual []byte) { + t.Helper() + decode := func(data []byte) any { + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var value any + require.NoError(t, decoder.Decode(&value)) + return value + } + require.Equal(t, decode(expected), decode(actual)) +} + func TestBuildRejectsInvalidMetadata(t *testing.T) { tests := []struct { name string @@ -1468,6 +1521,13 @@ func TestBuildRejectsInvalidMetadata(t *testing.T) { }}), errorText: `prompt "prompt" metadata`, }, + { + name: "invalid number", + builder: NewBuilder().SetTools([]ServerTool{{ + Tool: mcp.Tool{Name: "tool", Meta: mcp.Meta{"invalid": json.Number("nope")}}, + }}), + errorText: `tool "tool" metadata`, + }, } for _, tt := range tests { @@ -1475,7 +1535,11 @@ func TestBuildRejectsInvalidMetadata(t *testing.T) { _, err := tt.builder.Build() require.ErrorContains(t, err, "invalid inventory metadata") require.ErrorContains(t, err, tt.errorText) - require.ErrorContains(t, err, "unsupported type: chan int") + if tt.name == "invalid number" { + require.ErrorContains(t, err, "invalid number literal") + } else { + require.ErrorContains(t, err, "unsupported type: chan int") + } }) } } From ac54ad656bfdc4940ac0f26d45735c9c572d75dd Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 15:37:08 +0200 Subject: [PATCH 11/15] refactor(features): simplify lazy resolution Preserve string checker APIs, evaluate functional rules lazily through one request memo, remove metadata normalization and eager feature caches, and add remote-sized benchmarks. 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 | 4 +- cmd/github-mcp-server/generate_docs.go | 2 +- docs/feature-flags.md | 23 +- docs/insiders-features.md | 10 +- internal/ghmcp/server.go | 4 +- pkg/github/context_tools_test.go | 3 +- pkg/github/dependencies_test.go | 7 +- pkg/github/feature_flags_benchmark_test.go | 162 +++++++++++ pkg/github/feature_flags_test.go | 6 +- pkg/github/server_test.go | 2 +- pkg/github/tools_validation_test.go | 46 +-- pkg/http/handler.go | 2 +- pkg/http/handler_test.go | 12 +- pkg/http/server.go | 4 +- pkg/http/server_test.go | 2 +- pkg/inventory/builder.go | 76 +---- pkg/inventory/features.go | 158 ++++------- pkg/inventory/features_test.go | 86 ++++-- pkg/inventory/filters.go | 20 +- pkg/inventory/metadata.go | 60 ---- pkg/inventory/registry.go | 91 +++--- pkg/inventory/registry_test.go | 307 ++++----------------- 22 files changed, 427 insertions(+), 660 deletions(-) create mode 100644 pkg/github/feature_flags_benchmark_test.go delete mode 100644 pkg/inventory/metadata.go diff --git a/cmd/github-mcp-server/feature_flag_docs.go b/cmd/github-mcp-server/feature_flag_docs.go index 7c17e58222..e52237b138 100644 --- a/cmd/github-mcp-server/feature_flag_docs.go +++ b/cmd/github-mcp-server/feature_flag_docs.go @@ -100,8 +100,8 @@ func flaggedToolDiff(t translations.TranslationHelperFunc, flag string, defaultT // 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 inventory.FeatureFlag) (bool, error) { - return enabled[string(flag)], nil + checker := func(_ context.Context, flag string) (bool, error) { + return enabled[flag], nil } inv, _ := github.NewInventory(t). WithToolsets([]string{"all"}). diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 0d360ac1f6..a9a6c20f83 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, _ inventory.FeatureFlag) (bool, error) { +func noFeatureFlagsChecker(_ context.Context, _ string) (bool, error) { return false, nil } diff --git a/docs/feature-flags.md b/docs/feature-flags.md index e998527e85..f59a10518c 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -51,22 +51,25 @@ tool.FeatureRule = inventory.NewFeatureRule( ) ``` -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`. +Library consumers migrating existing inventory declarations should replace +`FeatureFlagEnable`, `FeatureFlagEnableAll`, and `FeatureFlagDisable` on +`ServerTool`, `ServerResourceTemplate`, and `ServerPrompt` with `FeatureRule`. +`FeatureFlagChecker` and `ToolDependencies.IsFeatureEnabled` continue to accept +string flag names. + +Rules are evaluated lazily after request narrowing. Normal Go short-circuiting +avoids checks that cannot affect the result, while one request-owned memo ensures +each flag actually reached is resolved at most once across tools, resources, +prompts, and `deps.IsFeatureEnabled`. Feature predicates are pure and may depend only on their resolver. Construction validates every combination of up to 16 declared flags, so an undeclared lookup fails immediately even when ordinary evaluation would short-circuit that branch. -The inventory's checker owns request feature state. Once installed, that state -is authoritative; a checker stored on tool dependencies is used only as a -fallback when handlers are invoked directly without request state. HTTP -availability is resolved after outer HTTP middleware and the inventory factory -run, but before MCP receiving middleware, because the tool set must be known -before constructing the MCP server. Handler-only lazy checks use the live -tool-call context. +The inventory's string-based checker owns request feature state. Once installed, +that state is authoritative; a checker stored on tool dependencies is used only +as a fallback when handlers are invoked directly without request state. --- diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 88c8d7ac53..4255e5bdd9 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -207,12 +207,10 @@ 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. +For tool availability, functional rules declare the flags they may read and are +evaluated lazily after request narrowing. Short-circuiting skips unnecessary +checks, and request-owned state memoizes each flag that is reached. The same +state backs `deps.IsFeatureEnabled`. `AllowedFeatureFlags` and `InsidersFeatureFlags` are deliberately independent sets: diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 91de092630..f713a44026 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -436,8 +436,8 @@ func RunStdioServer(cfg StdioServerConfig) error { // 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 inventory.FeatureFlag) (bool, error) { - return featureSet[string(flagName)], nil + return func(_ context.Context, flagName string) (bool, error) { + return featureSet[flagName], nil } } diff --git a/pkg/github/context_tools_test.go b/pkg/github/context_tools_test.go index 7ee3e16551..0825158abb 100644 --- a/pkg/github/context_tools_test.go +++ b/pkg/github/context_tools_test.go @@ -9,7 +9,6 @@ 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" @@ -190,7 +189,7 @@ func Test_GetMe_IFC_FeatureFlag(t *testing.T) { translations.NullTranslationHelper, FeatureFlags{}, 0, - func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { + func(_ context.Context, flagName string) (bool, error) { return flagName == FeatureFlagIFCLabels && enabled, nil }, stubExporters(), diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index c434921f58..223fc8aca4 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -14,7 +14,6 @@ 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" @@ -203,7 +202,7 @@ func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() // Create a feature checker that returns true for "test_flag" - checker := func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { + checker := func(_ context.Context, flagName string) (bool, error) { return flagName == "test_flag", nil } @@ -254,7 +253,7 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { t.Parallel() // Create a feature checker - checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) { + checker := func(_ context.Context, _ string) (bool, error) { return true, nil } @@ -389,7 +388,7 @@ func TestIsFeatureEnabled_CheckerError(t *testing.T) { t.Parallel() // Create a feature checker that returns an error - checker := func(_ context.Context, _ inventory.FeatureFlag) (bool, error) { + checker := func(_ context.Context, _ string) (bool, error) { return false, errors.New("checker error") } diff --git a/pkg/github/feature_flags_benchmark_test.go b/pkg/github/feature_flags_benchmark_test.go new file mode 100644 index 0000000000..5cb645bc22 --- /dev/null +++ b/pkg/github/feature_flags_benchmark_test.go @@ -0,0 +1,162 @@ +package github + +import ( + "context" + "fmt" + "sync/atomic" + "testing" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +func BenchmarkFeatureInventory(b *testing.B) { + for _, distribution := range featureBenchmarkDistributions() { + b.Run(distribution.name, func(b *testing.B) { + b.Run("build", func(b *testing.B) { + var calls atomic.Int64 + b.ReportAllocs() + for b.Loop() { + _, err := featureBenchmarkBuilder(distribution, &calls).Build() + if err != nil { + b.Fatal(err) + } + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + builder := featureBenchmarkBuilder(distribution, nil) + b.Run("preconstructed-builder", func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + if _, err := builder.Build(); err != nil { + b.Fatal(err) + } + } + }) + + b.Run("tools-list", func(b *testing.B) { + inv, calls := featureBenchmarkInventory(b, distribution) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + _ = inv.ForMCPRequest(inventory.MCPMethodToolsList, "").ToolsForRegistration(context.Background()) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + b.Run("unflagged-tool-call", func(b *testing.B) { + inv, calls := featureBenchmarkInventory(b, distribution) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + _ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "get_commit").ToolsForRegistration(context.Background()) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + b.Run("gated-tool-call", func(b *testing.B) { + inv, calls := featureBenchmarkInventory(b, distribution) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + _ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "get_file_blame").ToolsForRegistration(context.Background()) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + b.Run("ui-tool-call", func(b *testing.B) { + inv, calls := featureBenchmarkInventory(b, distribution) + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + _ = inv.ForMCPRequest(inventory.MCPMethodToolsCall, "ui_get").ToolsForRegistration(context.Background()) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + b.Run("direct-handler-checks", func(b *testing.B) { + var calls atomic.Int64 + checker := func(_ context.Context, flag string) (bool, error) { + calls.Add(1) + return distribution.enabled["*"] || distribution.enabled[flag], nil + } + b.ReportAllocs() + for b.Loop() { + ctx := inventory.WithFeatureState(context.Background(), checker) + _ = inventory.ResolveFeature(ctx, checker, inventory.FeatureFlag(FeatureFlagCSVOutput)) + _ = inventory.ResolveFeature(ctx, checker, inventory.FeatureFlag(FeatureFlagCSVOutput)) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + + b.Run("build-list-register", func(b *testing.B) { + var calls atomic.Int64 + b.ReportAllocs() + for b.Loop() { + inv, err := featureBenchmarkBuilder(distribution, &calls).Build() + if err != nil { + b.Fatal(err) + } + inv = inv.ForMCPRequest(inventory.MCPMethodToolsList, "") + server := mcp.NewServer(&mcp.Implementation{Name: "benchmark", Version: "v0"}, nil) + inv.RegisterAll(context.Background(), server, nil) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + }) + } +} + +type featureBenchmarkDistribution struct { + name string + enabled map[string]bool +} + +func featureBenchmarkDistributions() []featureBenchmarkDistribution { + return []featureBenchmarkDistribution{ + {name: "all-false"}, + { + name: "mixed", + enabled: map[string]bool{ + MCPAppsFeatureFlag: true, + FeatureFlagFileBlame: true, + FeatureFlagIssuesGranular: true, + FeatureFlagIssueDependencies: true, + }, + }, + {name: "all-true", enabled: map[string]bool{"*": true}}, + } +} + +func featureBenchmarkBuilder(distribution featureBenchmarkDistribution, calls *atomic.Int64) *inventory.Builder { + checker := func(_ context.Context, flag string) (bool, error) { + if calls != nil { + calls.Add(1) + } + return distribution.enabled["*"] || distribution.enabled[flag], nil + } + tools := AllTools(translations.NullTranslationHelper) + for i, baseCount := 0, len(tools); len(tools) < 139; i++ { + tool := tools[i%baseCount] + tool.Tool.Name = fmt.Sprintf("%s_remote_%d", tool.Tool.Name, i) + tools = append(tools, tool) + } + return inventory.NewBuilder(). + SetTools(tools). + SetResources(AllResources(translations.NullTranslationHelper)). + SetPrompts(AllPrompts(translations.NullTranslationHelper)). + WithToolsets([]string{"all"}). + WithFeatureChecker(checker) +} + +func featureBenchmarkInventory(b *testing.B, distribution featureBenchmarkDistribution) (*inventory.Inventory, *atomic.Int64) { + b.Helper() + var calls atomic.Int64 + inv, err := featureBenchmarkBuilder(distribution, &calls).Build() + if err != nil { + b.Fatal(err) + } + return inv, &calls +} diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 30282fab13..cc3fbf0837 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -20,11 +20,11 @@ import ( const RemoteMCPEnthusiasticGreeting inventory.FeatureFlag = "remote_mcp_enthusiastic_greeting" func featureCheckerFor(enabledFlags ...inventory.FeatureFlag) inventory.FeatureFlagChecker { - enabled := make(map[inventory.FeatureFlag]bool, len(enabledFlags)) + enabled := make(map[string]bool, len(enabledFlags)) for _, flag := range enabledFlags { - enabled[flag] = true + enabled[string(flag)] = true } - return func(_ context.Context, flagName inventory.FeatureFlag) (bool, error) { + return func(_ context.Context, flagName string) (bool, error) { return enabled[flagName], nil } } diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index e81247ac99..bd8b9abd6b 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -196,7 +196,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { func TestFeatureStateMiddlewareCachesHandlerChecks(t *testing.T) { var calls int - checker := func(_ context.Context, flag inventory.FeatureFlag) (bool, error) { + checker := func(_ context.Context, flag string) (bool, error) { calls++ return flag == "enabled", nil } diff --git a/pkg/github/tools_validation_test.go b/pkg/github/tools_validation_test.go index 5029bee2c9..3eacc8e1c2 100644 --- a/pkg/github/tools_validation_test.go +++ b/pkg/github/tools_validation_test.go @@ -169,25 +169,25 @@ func TestToolReadOnlyHintConsistency(t *testing.T) { // TestNoDuplicateToolNames ensures duplicate names cannot be enabled together. func TestNoDuplicateToolNames(t *testing.T) { tools := AllTools(stubTranslation) - rulesByName := make(map[string][]inventory.FeatureRule) + toolsByName := make(map[string][]inventory.ServerTool) for _, tool := range tools { - rulesByName[tool.Tool.Name] = append(rulesByName[tool.Tool.Name], tool.FeatureRule) + toolsByName[tool.Tool.Name] = append(toolsByName[tool.Tool.Name], tool) } // get_label is intentionally in both issues and labels toolsets for conformance // with original behavior where it was registered in both - for name, rules := range rulesByName { - if name == "get_label" || len(rules) < 2 { + for name, variants := range toolsByName { + if name == "get_label" || len(variants) < 2 { continue } - assert.False(t, featureRulesOverlap(rules), "tool variants for %q can be enabled together", name) + assert.False(t, featureDeclarationsOverlap(variants), "tool variants for %q can be enabled together", name) } } -func featureRulesOverlap(rules []inventory.FeatureRule) bool { +func featureDeclarationsOverlap(variants []inventory.ServerTool) bool { positions := make(map[inventory.FeatureFlag]uint) - for _, rule := range rules { - for _, feature := range rule.Features() { + for _, variant := range variants { + for _, feature := range variant.FeatureRule.Features() { if _, ok := positions[feature]; !ok { positions[feature] = uint(len(positions)) } @@ -199,10 +199,11 @@ func featureRulesOverlap(rules []inventory.FeatureRule) bool { for assignment := range 1 << len(positions) { enabled := 0 - for _, rule := range rules { - if rule.IsZero() || rule.Enabled(func(feature inventory.FeatureFlag) bool { - return assignment&(1< excluded - checker1 := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } + checker1 := func(_ context.Context, _ string) (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 FeatureFlag) (bool, error) { return flag == "new_feature", nil } + checker2 := func(_ context.Context, flag string) (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, _ FeatureFlag) (bool, error) { return true, nil } + checker3 := func(_ context.Context, _ string) (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)") @@ -1159,7 +1157,7 @@ func TestFeatureFlagError(t *testing.T) { } // Checker that returns error should treat as false (tool excluded) - checkerError := func(_ context.Context, _ FeatureFlag) (bool, error) { + checkerError := func(_ context.Context, _ string) (bool, error) { return false, fmt.Errorf("simulated error") } reg := mustBuild(t, NewBuilder().SetTools(tools).WithFeatureChecker(checkerError)) @@ -1189,7 +1187,7 @@ func TestFeatureFlagResources(t *testing.T) { } // With checker returning true, both should be included - checker := func(_ context.Context, _ FeatureFlag) (bool, error) { return true, nil } + checker := func(_ context.Context, _ string) (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()))) @@ -1216,7 +1214,7 @@ func TestFeatureFlagPrompts(t *testing.T) { } // With checker returning true, both should be included - checker := func(_ context.Context, _ FeatureFlag) (bool, error) { return true, nil } + checker := func(_ context.Context, _ string) (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()))) @@ -1236,7 +1234,7 @@ func TestRegisterAllSharesFeatureResolution(t *testing.T) { prompt.FeatureRule = rule calls := 0 - checker := func(context.Context, FeatureFlag) (bool, error) { + checker := func(context.Context, string) (bool, error) { calls++ return calls == 1, nil } @@ -1270,7 +1268,7 @@ func TestRegisterAllSharesFeatureResolution(t *testing.T) { require.Equal(t, 1, calls) } -func TestFeatureMetadataIsCachedAndNarrowed(t *testing.T) { +func TestRequiredFeaturesReflectNarrowedInventory(t *testing.T) { tools := []ServerTool{ mockToolWithFlags("tool_x", "toolset1", true, "x", ""), mockToolWithFlags("tool_y", "toolset1", true, "y", ""), @@ -1281,19 +1279,13 @@ func TestFeatureMetadataIsCachedAndNarrowed(t *testing.T) { require.Equal(t, []FeatureFlag{"x", "y"}, inv.RequiredFeatures()) - // RequiredFeatures reads cached metadata rather than walking live tools. - inv.tools[0].FeatureRule = NewFeatureRule([]FeatureFlag{"changed"}, func(featureAsBool FeatureResolver) bool { - return featureAsBool("changed") - }) - require.Equal(t, []FeatureFlag{"x", "y"}, inv.RequiredFeatures()) - - // Request narrowing builds a fresh cache over only the candidate variants. narrowed := inv.ForMCPRequest(MCPMethodToolsCall, "tool_x") - require.Equal(t, []FeatureFlag{"changed"}, narrowed.RequiredFeatures()) + require.Equal(t, []FeatureFlag{"x"}, narrowed.RequiredFeatures()) } -func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { - toolRule := NewFeatureRule([]FeatureFlag{"tool"}, func(featureAsBool FeatureResolver) bool { +func TestBuilderCopiesFeatureRuleItems(t *testing.T) { + toolFeatures := []FeatureFlag{"tool"} + toolRule := NewFeatureRule(toolFeatures, func(featureAsBool FeatureResolver) bool { return featureAsBool("tool") }) resourceRule := NewFeatureRule([]FeatureFlag{"resource"}, func(featureAsBool FeatureResolver) bool { @@ -1304,244 +1296,47 @@ func TestBuilderOwnsFeatureMetadataInputs(t *testing.T) { }) tools := []ServerTool{mockTool("tool", "toolset1", true)} tools[0].FeatureRule = toolRule - tools[0].Tool.Meta = nestedTestMeta("ui", "tool") resources := []ServerResourceTemplate{mockResource("resource", "toolset1", "uri")} resources[0].FeatureRule = resourceRule - resources[0].Template.Meta = nestedTestMeta("resource", "resource") prompts := []ServerPrompt{mockPrompt("prompt", "toolset1")} prompts[0].FeatureRule = promptRule - prompts[0].Prompt.Meta = nestedTestMeta("prompt", "prompt") - expectedToolMeta := mustMarshalJSON(t, tools[0].Tool.Meta) - expectedResourceMeta := mustMarshalJSON(t, resources[0].Template.Meta) - expectedPromptMeta := mustMarshalJSON(t, prompts[0].Prompt.Meta) - - checker := func(_ context.Context, flag FeatureFlag) (bool, error) { - return flag == "tool" || flag == "resource" || flag == "prompt" || flag == mcpAppsFeatureFlag, nil - } builder := NewBuilder(). SetTools(tools). SetResources(resources). SetPrompts(prompts). - WithToolsets([]string{"all"}). - WithFeatureChecker(checker) + WithToolsets([]string{"all"}) - mutateSourceTestMeta(tools[0].Tool.Meta, "ui", "after-set", 's') - mutateSourceTestMeta(resources[0].Template.Meta, "resource", "after-set", 's') - mutateSourceTestMeta(prompts[0].Prompt.Meta, "prompt", "after-set", 's') + toolFeatures[0] = "changed" + tools[0].FeatureRule = NewFeatureRule([]FeatureFlag{"changed"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("changed") + }) + resources[0].FeatureRule = tools[0].FeatureRule + prompts[0].FeatureRule = tools[0].FeatureRule inv := mustBuild(t, builder) - tools[0].FeatureRule.features[0] = "changed" - mutateSourceTestMeta(tools[0].Tool.Meta, "ui", "after-build", 'b') - resources[0].FeatureRule.features[0] = "changed" - mutateSourceTestMeta(resources[0].Template.Meta, "resource", "after-build", 'b') - prompts[0].FeatureRule.features[0] = "changed" - mutateSourceTestMeta(prompts[0].Prompt.Meta, "prompt", "after-build", 'b') - - require.Equal(t, []FeatureFlag{"prompt", "remote_mcp_ui_apps", "resource", "tool"}, inv.RequiredFeatures()) - availableTools := inv.AvailableTools(context.Background()) - availableResources := inv.AvailableResourceTemplates(context.Background()) - availablePrompts := inv.AvailablePrompts(context.Background()) - require.Len(t, availableTools, 1) - require.Len(t, availableResources, 1) - require.Len(t, availablePrompts, 1) - requireNestedTestMeta(t, availableTools[0].Tool.Meta, "ui", "tool") - requireNestedTestMeta(t, availableResources[0].Template.Meta, "resource", "resource") - requireNestedTestMeta(t, availablePrompts[0].Prompt.Meta, "prompt", "prompt") - requireExactTestMetaNumbers(t, availableTools[0].Tool.Meta, "ui") - requireExactTestMetaNumbers(t, availableResources[0].Template.Meta, "resource") - requireExactTestMetaNumbers(t, availablePrompts[0].Prompt.Meta, "prompt") - requireJSONEquivalent(t, expectedToolMeta, mustMarshalJSON(t, availableTools[0].Tool.Meta)) - requireJSONEquivalent(t, expectedResourceMeta, mustMarshalJSON(t, availableResources[0].Template.Meta)) - requireJSONEquivalent(t, expectedPromptMeta, mustMarshalJSON(t, availablePrompts[0].Prompt.Meta)) - - mutateNormalizedTestMeta(availableTools[0].Tool.Meta, "ui", "after-available") - availableTools[0].FeatureRule.features[0] = "changed" - mutateNormalizedTestMeta(availableResources[0].Template.Meta, "resource", "after-available") - availableResources[0].FeatureRule.features[0] = "changed" - mutateNormalizedTestMeta(availablePrompts[0].Prompt.Meta, "prompt", "after-available") - availablePrompts[0].FeatureRule.features[0] = "changed" - - allTools := inv.AllTools() - mutateNormalizedTestMeta(allTools[0].Tool.Meta, "ui", "after-all") - foundTool, _, err := inv.FindToolByName("tool") - require.NoError(t, err) - mutateNormalizedTestMeta(foundTool.Tool.Meta, "ui", "after-find") - - server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) - inv.RegisterAll(context.Background(), server, nil) - clientTransport, serverTransport := mcp.NewInMemoryTransports() - serverSession, err := server.Connect(context.Background(), serverTransport, nil) - require.NoError(t, err) - t.Cleanup(func() { _ = serverSession.Close() }) - client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) - clientSession, err := client.Connect(context.Background(), clientTransport, nil) - require.NoError(t, err) - t.Cleanup(func() { _ = clientSession.Close() }) - - registeredTools, err := clientSession.ListTools(context.Background(), nil) - require.NoError(t, err) - requireNestedTestMeta(t, registeredTools.Tools[0].Meta, "ui", "tool") - require.JSONEq(t, string(expectedToolMeta), string(mustMarshalJSON(t, registeredTools.Tools[0].Meta))) - registeredResources, err := clientSession.ListResourceTemplates(context.Background(), nil) - require.NoError(t, err) - requireNestedTestMeta(t, registeredResources.ResourceTemplates[0].Meta, "resource", "resource") - require.JSONEq(t, string(expectedResourceMeta), string(mustMarshalJSON(t, registeredResources.ResourceTemplates[0].Meta))) - registeredPrompts, err := clientSession.ListPrompts(context.Background(), nil) - require.NoError(t, err) - requireNestedTestMeta(t, registeredPrompts.Prompts[0].Meta, "prompt", "prompt") - require.JSONEq(t, string(expectedPromptMeta), string(mustMarshalJSON(t, registeredPrompts.Prompts[0].Meta))) -} -type testMetaMap map[string]string -type testMetaSlice []string -type testMetaPointer struct { - Value string `json:"value"` - Decimal float64 `json:"decimal"` + want := []FeatureFlag{"prompt", "resource", "tool"} + require.Equal(t, want, inv.RequiredFeatures()) } -const ( - testLargeSigned int64 = 9007199254740993 - testLargeUnsigned uint64 = 18446744073709551614 - testExponent = 6.022e23 - testDecimal = 1.2345678901234567 -) - -func nestedTestMeta(key, value string) mcp.Meta { - return mcp.Meta{ - key: map[string]any{ - "objects": []any{map[string]any{"value": value}}, - "strings": []string{value}, - "typed_map": testMetaMap{"value": value}, - "typed_slice": testMetaSlice{value}, - "bytes": []byte(value), - "pointer": &testMetaPointer{Value: value, Decimal: testDecimal}, - "numbers": &testMetaNumbers{ - Signed: testLargeSigned, - Unsigned: testLargeUnsigned, - Exponent: testExponent, - Decimal: testDecimal, - }, - }, +func TestMetadataBehaviorRemainsLive(t *testing.T) { + meta := mcp.Meta{ + "typed_map": map[string]string{"value": "original"}, + "bytes": []byte("original"), + "invalid": make(chan int), } -} - -type testMetaNumbers struct { - Signed int64 `json:"signed"` - Unsigned uint64 `json:"unsigned"` - Exponent float64 `json:"exponent"` - Decimal float64 `json:"decimal"` -} - -func mutateSourceTestMeta(meta mcp.Meta, key, value string, marker byte) { - nested := meta[key].(map[string]any) - nested["objects"].([]any)[0].(map[string]any)["value"] = value - nested["strings"].([]string)[0] = value - nested["typed_map"].(testMetaMap)["value"] = value - nested["typed_slice"].(testMetaSlice)[0] = value - nested["bytes"].([]byte)[0] = marker - nested["pointer"].(*testMetaPointer).Value = value - nested["numbers"].(*testMetaNumbers).Signed = 1 -} - -func mutateNormalizedTestMeta(meta mcp.Meta, key, value string) { - nested := meta[key].(map[string]any) - nested["objects"].([]any)[0].(map[string]any)["value"] = value - nested["strings"].([]any)[0] = value - nested["typed_map"].(map[string]any)["value"] = value - nested["typed_slice"].([]any)[0] = value - nested["bytes"] = value - nested["pointer"].(map[string]any)["value"] = value - nested["numbers"].(map[string]any)["signed"] = json.Number("1") -} - -func requireNestedTestMeta(t *testing.T, meta mcp.Meta, key, value string) { - t.Helper() - nested := meta[key].(map[string]any) - require.Equal(t, value, nested["objects"].([]any)[0].(map[string]any)["value"]) - require.Equal(t, value, nested["strings"].([]any)[0]) - require.Equal(t, value, nested["typed_map"].(map[string]any)["value"]) - require.Equal(t, value, nested["typed_slice"].([]any)[0]) - require.Equal(t, base64.StdEncoding.EncodeToString([]byte(value)), nested["bytes"]) - require.Equal(t, value, nested["pointer"].(map[string]any)["value"]) -} - -func requireExactTestMetaNumbers(t *testing.T, meta mcp.Meta, key string) { - t.Helper() - nested := meta[key].(map[string]any) - require.Equal(t, json.Number("1.2345678901234567"), nested["pointer"].(map[string]any)["decimal"]) - numbers := nested["numbers"].(map[string]any) - require.Equal(t, json.Number("9007199254740993"), numbers["signed"]) - require.Equal(t, json.Number("18446744073709551614"), numbers["unsigned"]) - require.Equal(t, json.Number("6.022e+23"), numbers["exponent"]) - require.Equal(t, json.Number("1.2345678901234567"), numbers["decimal"]) -} - -func mustMarshalJSON(t *testing.T, value any) []byte { - t.Helper() - data, err := json.Marshal(value) + tool := mockTool("tool", "toolset1", true) + tool.Tool.Meta = meta + inv, err := NewBuilder().SetTools([]ServerTool{tool}).Build() require.NoError(t, err) - return data -} - -func requireJSONEquivalent(t *testing.T, expected, actual []byte) { - t.Helper() - decode := func(data []byte) any { - decoder := json.NewDecoder(bytes.NewReader(data)) - decoder.UseNumber() - var value any - require.NoError(t, decoder.Decode(&value)) - return value - } - require.Equal(t, decode(expected), decode(actual)) -} -func TestBuildRejectsInvalidMetadata(t *testing.T) { - tests := []struct { - name string - builder *Builder - errorText string - }{ - { - name: "tool", - builder: NewBuilder().SetTools([]ServerTool{{ - Tool: mcp.Tool{Name: "tool", Meta: mcp.Meta{"invalid": make(chan int)}}, - }}), - errorText: `tool "tool" metadata`, - }, - { - name: "resource", - builder: NewBuilder().SetResources([]ServerResourceTemplate{{ - Template: mcp.ResourceTemplate{Name: "resource", Meta: mcp.Meta{"invalid": make(chan int)}}, - }}), - errorText: `resource template "resource" metadata`, - }, - { - name: "prompt", - builder: NewBuilder().SetPrompts([]ServerPrompt{{ - Prompt: mcp.Prompt{Name: "prompt", Meta: mcp.Meta{"invalid": make(chan int)}}, - }}), - errorText: `prompt "prompt" metadata`, - }, - { - name: "invalid number", - builder: NewBuilder().SetTools([]ServerTool{{ - Tool: mcp.Tool{Name: "tool", Meta: mcp.Meta{"invalid": json.Number("nope")}}, - }}), - errorText: `tool "tool" metadata`, - }, - } + meta["ui"] = map[string]any{"resourceUri": "ui://live"} + meta["typed_map"].(map[string]string)["value"] = "changed" + meta["bytes"].([]byte)[0] = 'X' - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, err := tt.builder.Build() - require.ErrorContains(t, err, "invalid inventory metadata") - require.ErrorContains(t, err, tt.errorText) - if tt.name == "invalid number" { - require.ErrorContains(t, err, "invalid number literal") - } else { - require.ErrorContains(t, err, "unsupported type: chan int") - } - }) - } + require.Contains(t, inv.RequiredFeatures(), mcpAppsFeatureFlag) + available := inv.AllTools() + require.Equal(t, "changed", available[0].Tool.Meta["typed_map"].(map[string]string)["value"]) + require.Equal(t, byte('X'), available[0].Tool.Meta["bytes"].([]byte)[0]) } func TestServerToolHasHandler(t *testing.T) { @@ -1819,7 +1614,7 @@ func TestEnabledAndFeatureFlagInteraction(t *testing.T) { } // Feature flag not enabled - tool should be excluded despite Enabled returning true - checkerOff := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } reg1 := mustBuild(t, NewBuilder(). SetTools([]ServerTool{tool}). WithToolsets([]string{"all"}). @@ -1830,7 +1625,7 @@ func TestEnabledAndFeatureFlagInteraction(t *testing.T) { } // Feature flag enabled - tool should be included - checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + checker := func(_ context.Context, flag string) (bool, error) { return flag == "my_feature", nil } reg2 := mustBuild(t, NewBuilder(). @@ -1889,7 +1684,7 @@ func TestAllFiltersInteraction(t *testing.T) { return true, nil } - checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + checker := func(_ context.Context, flag string) (bool, error) { return flag == "my_feature", nil } @@ -2007,7 +1802,7 @@ func TestFilteringOrder(t *testing.T) { return true, nil } - checker := func(_ context.Context, _ FeatureFlag) (bool, error) { + checker := func(_ context.Context, _ string) (bool, error) { callOrder = append(callOrder, "FeatureFlag") return true, nil } @@ -2024,8 +1819,8 @@ func TestFilteringOrder(t *testing.T) { _ = reg.AvailableTools(context.Background()) - // Declared features resolve first, then Enabled runs before read-only stops. - expectedOrder := []string{"FeatureFlag", "Enabled"} + // Enabled runs before the lazy feature check; read-only then stops the pipeline. + expectedOrder := []string{"Enabled", "FeatureFlag"} if len(callOrder) != len(expectedOrder) { t.Errorf("Expected %d checks, got %d: %v", len(expectedOrder), len(callOrder), callOrder) } @@ -2048,7 +1843,7 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { } // Test 1: Flag is OFF - first tool variant should be available - checkerOff := func(_ context.Context, _ FeatureFlag) (bool, error) { return false, nil } + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). WithToolsets([]string{"all"}). @@ -2063,7 +1858,7 @@ func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { } // Test 2: Flag is ON - second tool variant should be available - checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + checker := func(_ context.Context, flag string) (bool, error) { return flag == "consolidated_flag", nil } regFlagOn := mustBuild(t, NewBuilder(). @@ -2100,7 +1895,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, _ FeatureFlag) (bool, error) { return false, nil } + checkerOff := func(_ context.Context, _ string) (bool, error) { return false, nil } regFlagOff := mustBuild(t, NewBuilder(). SetTools(tools). WithDeprecatedAliases(deprecatedAliases). @@ -2116,7 +1911,7 @@ func TestWithTools_DeprecatedAliasAndFeatureFlag(t *testing.T) { } // Test 2: Flag ON - new_tool should be available via alias resolution - checker := func(_ context.Context, flag FeatureFlag) (bool, error) { + checker := func(_ context.Context, flag string) (bool, error) { return flag == "my_flag", nil } regFlagOn := mustBuild(t, NewBuilder(). @@ -2179,8 +1974,8 @@ func TestWithMCPApps_EnabledPreservesUIMetadata(t *testing.T) { }) // Feature checker enables MCP Apps - UI meta should be preserved - mcpAppsChecker := func(_ context.Context, flag FeatureFlag) (bool, error) { - return flag == mcpAppsFeatureFlag, nil + mcpAppsChecker := func(_ context.Context, flag string) (bool, error) { + return flag == string(mcpAppsFeatureFlag), nil } reg := mustBuild(t, NewBuilder(). SetTools([]ServerTool{toolWithUI}). From 875a1f69caa2ffe1da5e817875a9c108cc5a52e6 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 2 Sep 2026 17:58:57 +0200 Subject: [PATCH 12/15] fix(features): fail nested checker resolution closed Disallow recursive ResolveFeature calls from feature checkers so direct, negating, multi-node, and concurrent cycles cannot cache enabled results or wait on one another. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- docs/feature-flags.md | 2 ++ pkg/inventory/features.go | 24 ++++++++++++------------ pkg/inventory/features_test.go | 26 ++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index f59a10518c..568cf84542 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -70,6 +70,8 @@ branch. The inventory's string-based checker owns request feature state. Once installed, that state is authoritative; a checker stored on tool dependencies is used only as a fallback when handlers are invoked directly without request state. +Feature checkers must not call `ResolveFeature`; nested resolution fails the +owning check closed. --- diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index 175dd46916..6ae7126117 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -13,9 +13,8 @@ const maxFeatureRuleFlags = 16 // FeatureFlag identifies a feature consistently across inventory consumers. type FeatureFlag string -// FeatureFlagChecker resolves one feature flag for the current request. Every -// context value needed for availability checks must be installed before the -// inventory is resolved. Handler-only checks receive the live tool-call context. +// FeatureFlagChecker resolves one feature flag for the current request. Checkers +// must not call ResolveFeature; nested resolution fails the owning check closed. type FeatureFlagChecker func(ctx context.Context, flag string) (bool, error) // FeatureResolver returns the resolved value of a feature flag. @@ -154,6 +153,16 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool { } owner := resolvingFeatureFromContext(ctx) + if owner != nil { + s.mu.Lock() + if ownerResult := s.results[owner.flag]; ownerResult != nil { + ownerResult.failed = true + } + s.mu.Unlock() + fmt.Fprintf(os.Stderr, "Feature flag checker attempted nested resolution of %q\n", feature) + return false + } + s.mu.Lock() if s.results == nil { s.results = make(map[FeatureFlag]*featureResult) @@ -165,15 +174,6 @@ func (s *featureState) enabled(ctx context.Context, feature FeatureFlag) bool { s.mu.Unlock() return enabled } - if owner != nil { - result.failed = true - if ownerResult := s.results[owner.flag]; ownerResult != nil { - ownerResult.failed = true - } - s.mu.Unlock() - fmt.Fprintf(os.Stderr, "Feature flag resolution cycle detected for %q\n", feature) - return false - } for !result.done { s.cond.Wait() } diff --git a/pkg/inventory/features_test.go b/pkg/inventory/features_test.go index 4a1dbc4127..1170e866ec 100644 --- a/pkg/inventory/features_test.go +++ b/pkg/inventory/features_test.go @@ -146,7 +146,7 @@ func TestLazyFeatureResolutionUsesLiveContext(t *testing.T) { assert.True(t, ResolveFeature(ctx, checker, "handler_only")) } -func TestFeatureResolutionIsReentrantAcrossFlags(t *testing.T) { +func TestNestedFeatureResolutionFailsClosed(t *testing.T) { var checker FeatureFlagChecker checker = func(ctx context.Context, flag string) (bool, error) { if flag == "meta" { @@ -156,7 +156,8 @@ func TestFeatureResolutionIsReentrantAcrossFlags(t *testing.T) { } ctx := WithFeatureState(context.Background(), checker) - assert.True(t, ResolveFeature(ctx, nil, "meta")) + assert.False(t, ResolveFeature(ctx, nil, "meta")) + assert.True(t, ResolveFeature(ctx, nil, "base")) } func TestDirectFeatureResolutionCycleFailsClosed(t *testing.T) { @@ -197,6 +198,27 @@ func TestMutualFeatureResolutionCycleFailsClosed(t *testing.T) { assert.False(t, ResolveFeature(ctx, nil, "b")) } +func TestThreeNodeFeatureResolutionCycleFailsClosed(t *testing.T) { + var checker FeatureFlagChecker + checker = func(ctx context.Context, flag string) (bool, error) { + switch flag { + case "a": + return !ResolveFeature(ctx, checker, "b"), nil + case "b": + return !ResolveFeature(ctx, checker, "c"), nil + case "c": + return !ResolveFeature(ctx, checker, "a"), nil + default: + return false, nil + } + } + + ctx := WithFeatureState(context.Background(), checker) + assert.False(t, ResolveFeature(ctx, nil, "a")) + assert.False(t, ResolveFeature(ctx, nil, "b")) + assert.False(t, ResolveFeature(ctx, nil, "c")) +} + func TestConcurrentFeatureResolutionIsDeduplicated(t *testing.T) { var ( calls int From 0baddbf8ea18842fa8ea342761a2f033aa20e977 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 3 Sep 2026 09:55:50 +0200 Subject: [PATCH 13/15] docs(features): describe lazy rule resolution Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/features.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index 6ae7126117..bd33b2197b 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -26,8 +26,8 @@ type FeatureResolver func(flag FeatureFlag) bool 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. +// The predicate resolves reached flags lazily with normal Go boolean semantics, +// while request state deduplicates repeated checks. type FeatureRule struct { features []FeatureFlag predicate FeaturePredicate From 7f8940238828eab4f3075a3f2e1d4e9ad693b1d4 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 3 Sep 2026 13:46:45 +0200 Subject: [PATCH 14/15] perf(features): filter static exclusions first Skip feature checks for read-only, filtered, unselected, protocol-ineligible, and capability-ineligible tools, and reject declared rules without predicates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- docs/feature-flags.md | 8 +- pkg/context/mcp_info.go | 6 + pkg/github/feature_flags_benchmark_test.go | 26 ++++ pkg/http/middleware/mcp_parse.go | 13 +- pkg/http/middleware/mcp_parse_test.go | 42 ++++-- pkg/inventory/features.go | 3 + pkg/inventory/features_test.go | 7 + pkg/inventory/filters.go | 49 +++---- pkg/inventory/registry_test.go | 143 ++++++++++++++++++++- 9 files changed, 253 insertions(+), 44 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 568cf84542..571d469af5 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -57,10 +57,10 @@ Library consumers migrating existing inventory declarations should replace `FeatureFlagChecker` and `ToolDependencies.IsFeatureEnabled` continue to accept string flag names. -Rules are evaluated lazily after request narrowing. Normal Go short-circuiting -avoids checks that cannot affect the result, while one request-owned memo ensures -each flag actually reached is resolved at most once across tools, resources, -prompts, and `deps.IsFeatureEnabled`. +Rules are evaluated lazily after request narrowing and static availability +filters. Normal Go short-circuiting avoids checks that cannot affect the result, +while one request-owned memo ensures each flag actually reached is resolved at +most once across tools, resources, prompts, and `deps.IsFeatureEnabled`. Feature predicates are pure and may depend only on their resolver. Construction validates every combination of up to 16 declared flags, so an undeclared lookup diff --git a/pkg/context/mcp_info.go b/pkg/context/mcp_info.go index af474b13a0..fc09e6b139 100644 --- a/pkg/context/mcp_info.go +++ b/pkg/context/mcp_info.go @@ -3,6 +3,8 @@ package context import ( "context" "encoding/json" + + "github.com/modelcontextprotocol/go-sdk/mcp" ) type mcpMethodInfoCtx string @@ -22,6 +24,10 @@ type MCPMethodInfo struct { ItemName string // RawArguments contains the unmaterialized tool arguments for tools/call requests. RawArguments json.RawMessage + // ProtocolVersion and ClientCapabilities describe the requesting MCP client + // when stateless HTTP parsing makes them available before registration. + ProtocolVersion string + ClientCapabilities *mcp.ClientCapabilities } // DecodeArguments materializes tool arguments when request middleware needs diff --git a/pkg/github/feature_flags_benchmark_test.go b/pkg/github/feature_flags_benchmark_test.go index 5cb645bc22..79aa17d55d 100644 --- a/pkg/github/feature_flags_benchmark_test.go +++ b/pkg/github/feature_flags_benchmark_test.go @@ -46,6 +46,32 @@ func BenchmarkFeatureInventory(b *testing.B) { b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") }) + b.Run("read-only-tools-list", func(b *testing.B) { + var calls atomic.Int64 + checker := func(_ context.Context, flag string) (bool, error) { + calls.Add(1) + return distribution.enabled["*"] || distribution.enabled[flag], nil + } + tools := AllTools(translations.NullTranslationHelper) + inv, err := inventory.NewBuilder(). + SetTools(tools). + SetResources(AllResources(translations.NullTranslationHelper)). + SetPrompts(AllPrompts(translations.NullTranslationHelper)). + WithToolsets([]string{"all"}). + WithReadOnly(true). + WithFeatureChecker(checker). + Build() + if err != nil { + b.Fatal(err) + } + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + _ = inv.ForMCPRequest(inventory.MCPMethodToolsList, "").ToolsForRegistration(context.Background()) + } + b.ReportMetric(float64(calls.Load())/float64(b.N), "checks/op") + }) + b.Run("unflagged-tool-call", func(b *testing.B) { inv, calls := featureBenchmarkInventory(b, distribution) b.ReportAllocs() diff --git a/pkg/http/middleware/mcp_parse.go b/pkg/http/middleware/mcp_parse.go index 0b56902261..bcc44f01f5 100644 --- a/pkg/http/middleware/mcp_parse.go +++ b/pkg/http/middleware/mcp_parse.go @@ -7,6 +7,7 @@ import ( "net/http" ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/modelcontextprotocol/go-sdk/mcp" ) // mcpJSONRPCRequest represents the structure of an MCP JSON-RPC request. @@ -21,7 +22,11 @@ type mcpJSONRPCRequest struct { // For prompts/get // Name is shared with tools/call // For resources/read - URI string `json:"uri,omitempty"` + URI string `json:"uri,omitempty"` + Meta struct { + ProtocolVersion string `json:"io.modelcontextprotocol/protocolVersion,omitempty"` + ClientCapabilities *mcp.ClientCapabilities `json:"io.modelcontextprotocol/clientCapabilities,omitempty"` + } `json:"_meta"` } `json:"params"` } @@ -101,7 +106,11 @@ func parseMCPMethodInfo(body []byte) (*ghcontext.MCPMethodInfo, error) { return nil, nil } - methodInfo := &ghcontext.MCPMethodInfo{Method: mcpReq.Method} + methodInfo := &ghcontext.MCPMethodInfo{ + Method: mcpReq.Method, + ProtocolVersion: mcpReq.Params.Meta.ProtocolVersion, + ClientCapabilities: mcpReq.Params.Meta.ClientCapabilities, + } switch mcpReq.Method { case "tools/call": methodInfo.ItemName = mcpReq.Params.Name diff --git a/pkg/http/middleware/mcp_parse_test.go b/pkg/http/middleware/mcp_parse_test.go index e067f7808a..8034dc29a6 100644 --- a/pkg/http/middleware/mcp_parse_test.go +++ b/pkg/http/middleware/mcp_parse_test.go @@ -9,22 +9,25 @@ import ( "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestWithMCPParse(t *testing.T) { tests := []struct { - name string - method string - path string - body string - expectInfo bool - expectedMethod string - expectedItem string - expectedRaw string - expectedArgs map[string]any - expectArgsError bool + name string + method string + path string + body string + expectInfo bool + expectedMethod string + expectedItem string + expectedRaw string + expectedArgs map[string]any + expectedProtocol string + expectedForm bool + expectArgsError bool }{ { name: "health check path is skipped", @@ -76,6 +79,19 @@ func TestWithMCPParse(t *testing.T) { expectInfo: true, expectedMethod: "tools/list", }, + { + name: "tools/list parses client availability", + method: http.MethodPost, + path: "/mcp", + body: `{"jsonrpc":"2.0","method":"tools/list","params":{"_meta":{ + "io.modelcontextprotocol/protocolVersion":"2026-07-28", + "io.modelcontextprotocol/clientCapabilities":{"elicitation":{"form":{}}} + }}}`, + expectInfo: true, + expectedMethod: "tools/list", + expectedProtocol: "2026-07-28", + expectedForm: true, + }, { name: "tools/call parses name", method: http.MethodPost, @@ -158,6 +174,12 @@ func TestWithMCPParse(t *testing.T) { require.NotNil(t, capturedInfo) assert.Equal(t, tt.expectedMethod, capturedInfo.Method) assert.Equal(t, tt.expectedItem, capturedInfo.ItemName) + assert.Equal(t, tt.expectedProtocol, capturedInfo.ProtocolVersion) + if tt.expectedForm { + require.NotNil(t, capturedInfo.ClientCapabilities) + require.NotNil(t, capturedInfo.ClientCapabilities.Elicitation) + assert.Equal(t, &mcp.FormElicitationCapabilities{}, capturedInfo.ClientCapabilities.Elicitation.Form) + } if tt.expectedRaw != "" { assert.JSONEq(t, tt.expectedRaw, string(capturedInfo.RawArguments)) } diff --git a/pkg/inventory/features.go b/pkg/inventory/features.go index bd33b2197b..25665fde82 100644 --- a/pkg/inventory/features.go +++ b/pkg/inventory/features.go @@ -49,6 +49,9 @@ func NewFeatureRule(features []FeatureFlag, predicate FeaturePredicate) FeatureR features: declared, predicate: predicate, } + if len(declared) > 0 && predicate == nil { + panic("feature rule declares flags without a predicate") + } rule.validate() return rule } diff --git a/pkg/inventory/features_test.go b/pkg/inventory/features_test.go index 1170e866ec..72316a687c 100644 --- a/pkg/inventory/features_test.go +++ b/pkg/inventory/features_test.go @@ -71,6 +71,13 @@ func TestFeatureRuleRejectsEmptyFeature(t *testing.T) { }) } +func TestFeatureRuleRejectsDeclaredFlagsWithoutPredicate(t *testing.T) { + assert.PanicsWithValue(t, "feature rule declares flags without a predicate", func() { + NewFeatureRule([]FeatureFlag{"declared"}, nil) + }) + assert.True(t, FeatureRule{}.IsZero()) +} + func TestFeatureStateDeduplicatesChecks(t *testing.T) { calls := make(map[FeatureFlag]int) checker := func(_ context.Context, flag string) (bool, error) { diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 7eeb715d7e..1e65b55685 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "sort" + + ghcontext "github.com/github/github-mcp-server/pkg/context" ) // isToolsetEnabled checks if a toolset is enabled based on current filters. @@ -25,12 +27,11 @@ func (r *Inventory) checkFeatureFlag(ctx context.Context, flagName FeatureFlag) // isToolEnabled checks if a specific tool is enabled based on current filters. // Filter evaluation order: // 1. Tool.Enabled (tool self-filtering) -// 2. Functional feature rule -// 3. Read-only filter -// 4. Builder filters (via WithFilter) -// 5. Toolset/additional tools +// 2. Read-only and builder filters +// 3. Toolset/additional and MCP availability filters +// 4. Functional feature rule func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, featureAsBool FeatureResolver) bool { - // 1. Check tool's own Enabled function first + // 1. Check tool's own Enabled function. if tool.Enabled != nil { enabled, err := tool.Enabled(ctx) if err != nil { @@ -41,15 +42,10 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, feature return false } } - // 2. Check feature availability. - if r.featureChecker != nil && !tool.FeatureRule.Enabled(featureAsBool) { - return false - } - // 3. Check read-only filter (applies to all tools) + // 2. Apply static inventory filters. if r.readOnly && !tool.IsReadOnly() { return false } - // 4. Apply builder filters. for _, filter := range r.filters { allowed, err := filter(ctx, tool) if err != nil { @@ -60,12 +56,19 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, feature return false } } - // 5. Check if tool is in additionalTools (bypasses toolset filter) - if r.additionalTools != nil && r.additionalTools[tool.Tool.Name] { - return true + // 3. Apply selection and request-static MCP availability. + if (r.additionalTools == nil || !r.additionalTools[tool.Tool.Name]) && !r.isToolsetEnabled(tool.Toolset.ID) { + return false + } + if availability := tool.availability(); !availability.unrestricted() { + if info, ok := ghcontext.MCPMethod(ctx); ok && + (info.Method == MCPMethodToolsList || info.Method == MCPMethodToolsCall) && + !toolAvailable(info.ProtocolVersion, info.ClientCapabilities, availability) { + return false + } } - // 6. Check toolset filter - if !r.isToolsetEnabled(tool.Toolset.ID) { + // 4. Check feature availability. + if r.featureChecker != nil && !tool.FeatureRule.Enabled(featureAsBool) { return false } return true @@ -136,12 +139,13 @@ func (r *Inventory) availableResourceTemplates(ctx context.Context) []ServerReso var result []ServerResourceTemplate for i := range r.resourceTemplates { res := &r.resourceTemplates[i] - if r.featureChecker != nil && !res.FeatureRule.Enabled(featureAsBool) { + if !r.isToolsetEnabled(res.Toolset.ID) { continue } - if r.isToolsetEnabled(res.Toolset.ID) { - result = append(result, *res) + if r.featureChecker != nil && !res.FeatureRule.Enabled(featureAsBool) { + continue } + result = append(result, *res) } // Sort deterministically: by toolset ID, then by template name @@ -170,12 +174,13 @@ func (r *Inventory) availablePrompts(ctx context.Context) []ServerPrompt { var result []ServerPrompt for i := range r.prompts { prompt := &r.prompts[i] - if r.featureChecker != nil && !prompt.FeatureRule.Enabled(featureAsBool) { + if !r.isToolsetEnabled(prompt.Toolset.ID) { continue } - if r.isToolsetEnabled(prompt.Toolset.ID) { - result = append(result, *prompt) + if r.featureChecker != nil && !prompt.FeatureRule.Enabled(featureAsBool) { + continue } + result = append(result, *prompt) } // Sort deterministically: by toolset ID, then by prompt name diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 9b18f8c2fb..818692d189 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "testing" + "time" ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -1784,10 +1785,9 @@ func TestFilteredToolsMatchesAvailableTools(t *testing.T) { func TestFilteringOrder(t *testing.T) { // Test that filters are applied in the correct order: // 1. Tool.Enabled - // 2. Feature rule - // 3. Read-only - // 4. Builder filters - // 5. Toolset/additional tools + // 2. Read-only and builder filters + // 3. Toolset and MCP availability filters + // 4. Feature rule callOrder := []string{} @@ -1819,8 +1819,7 @@ func TestFilteringOrder(t *testing.T) { _ = reg.AvailableTools(context.Background()) - // Enabled runs before the lazy feature check; read-only then stops the pipeline. - expectedOrder := []string{"Enabled", "FeatureFlag"} + expectedOrder := []string{"Enabled"} if len(callOrder) != len(expectedOrder) { t.Errorf("Expected %d checks, got %d: %v", len(expectedOrder), len(callOrder), callOrder) } @@ -1832,6 +1831,138 @@ func TestFilteringOrder(t *testing.T) { } } +func TestReadOnlySkipsWriteToolFeatureChecks(t *testing.T) { + writeTool := mockToolWithFlags("write_tool", "toolset1", false, "write_feature", "") + readTool := mockTool("read_tool", "toolset1", true) + calls := 0 + checker := func(_ context.Context, _ string) (bool, error) { + calls++ + return true, nil + } + + inv := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{writeTool, readTool}). + WithToolsets([]string{"all"}). + WithReadOnly(true). + WithFeatureChecker(checker)) + + available := inv.AvailableTools(context.Background()) + require.Len(t, available, 1) + require.Equal(t, "read_tool", available[0].Tool.Name) + require.Zero(t, calls) +} + +func TestStaticFiltersRunBeforeFeatureChecks(t *testing.T) { + rule := func(feature FeatureFlag) FeatureRule { + return NewFeatureRule([]FeatureFlag{feature}, func(featureAsBool FeatureResolver) bool { + if !featureAsBool(feature) { + return false + } + return featureAsBool(feature) + }) + } + readOnlyExcluded := mockTool("write", "enabled", false) + readOnlyExcluded.FeatureRule = rule("write") + toolsetExcluded := mockTool("wrong_toolset", "disabled", true) + toolsetExcluded.FeatureRule = rule("wrong_toolset") + filterExcluded := mockTool("filtered", "enabled", true) + filterExcluded.FeatureRule = rule("filtered") + enabledExcluded := mockTool("enabled_false", "enabled", true) + enabledExcluded.FeatureRule = rule("enabled_false") + enabledExcluded.Enabled = func(context.Context) (bool, error) { return false, nil } + protocolExcluded := mockTool("protocol", "enabled", true) + protocolExcluded.FeatureRule = rule("protocol") + protocolExcluded.MinimumProtocolVersion = ProtocolVersionMultiRoundTrip + elicitationExcluded := mockTool("elicitation", "enabled", true) + elicitationExcluded.FeatureRule = rule("elicitation") + elicitationExcluded.RequiredElicitationMode = ElicitationModeForm + survivor := mockTool("survivor", "enabled", true) + survivor.FeatureRule = rule("survivor") + resource := mockResource("resource", "disabled", "test://resource") + resource.FeatureRule = rule("resource") + prompt := mockPrompt("prompt", "disabled") + prompt.FeatureRule = rule("prompt") + + calls := make(map[string]int) + checker := func(_ context.Context, feature string) (bool, error) { + time.Sleep(time.Millisecond) + calls[feature]++ + return true, nil + } + inv := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{ + readOnlyExcluded, + toolsetExcluded, + filterExcluded, + enabledExcluded, + protocolExcluded, + elicitationExcluded, + survivor, + }). + SetResources([]ServerResourceTemplate{resource}). + SetPrompts([]ServerPrompt{prompt}). + WithToolsets([]string{"enabled"}). + WithReadOnly(true). + WithFeatureChecker(checker). + WithFilter(func(_ context.Context, tool *ServerTool) (bool, error) { + return tool.Tool.Name != "filtered", nil + })) + + ctx := ghcontext.WithMCPMethodInfo(context.Background(), &ghcontext.MCPMethodInfo{ + Method: MCPMethodToolsList, + ProtocolVersion: "2025-11-25", + ClientCapabilities: &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{URL: &mcp.URLElicitationCapabilities{}}}, + }) + require.Len(t, inv.AvailableTools(ctx), 1) + require.Empty(t, inv.AvailableResourceTemplates(ctx)) + require.Empty(t, inv.AvailablePrompts(ctx)) + require.Equal(t, map[string]int{"survivor": 1}, calls) +} + +func TestMCPAvailabilityRunsBeforeFeatureChecks(t *testing.T) { + rule := NewFeatureRule([]FeatureFlag{"feature"}, func(featureAsBool FeatureResolver) bool { + return featureAsBool("feature") + }) + protocolTool := mockTool("protocol", "toolset1", true) + protocolTool.FeatureRule = rule + protocolTool.MinimumProtocolVersion = ProtocolVersionMultiRoundTrip + elicitationTool := mockTool("elicitation", "toolset1", true) + elicitationTool.FeatureRule = rule + elicitationTool.RequiredElicitationMode = ElicitationModeForm + + for _, method := range []string{MCPMethodToolsList, MCPMethodToolsCall} { + t.Run(method, func(t *testing.T) { + calls := 0 + checker := func(_ context.Context, _ string) (bool, error) { + calls++ + return true, nil + } + inv := mustBuild(t, NewBuilder(). + SetTools([]ServerTool{protocolTool, elicitationTool}). + WithToolsets([]string{"all"}). + WithFeatureChecker(checker)) + ctx := ghcontext.WithMCPMethodInfo(context.Background(), &ghcontext.MCPMethodInfo{ + Method: method, + ProtocolVersion: "2025-11-25", + ClientCapabilities: &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{URL: &mcp.URLElicitationCapabilities{}}}, + }) + + require.Empty(t, inv.AvailableTools(ctx)) + require.Zero(t, calls) + + ctx = ghcontext.WithMCPMethodInfo(context.Background(), &ghcontext.MCPMethodInfo{ + Method: method, + ProtocolVersion: ProtocolVersionMultiRoundTrip, + ClientCapabilities: &mcp.ClientCapabilities{ + Elicitation: &mcp.ElicitationCapabilities{Form: &mcp.FormElicitationCapabilities{}}, + }, + }) + require.Len(t, inv.AvailableTools(ctx), 2) + require.Equal(t, 1, calls) + }) + } +} + func TestForMCPRequest_ToolsCall_FeatureFlaggedVariants(t *testing.T) { // Simulate the get_job_logs scenario: two tools with the same name but different feature flags // - one "get_job_logs" variant available when the flag is off From 0d8baadcfe37a4f083d53ef070e7700300429e16 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 3 Sep 2026 13:50:20 +0200 Subject: [PATCH 15/15] fix(features): defer unknown client availability Apply pre-feature protocol and elicitation filtering only when stateless request metadata is known, leaving existing middleware to enforce unknown contexts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e4a1ca6-53f7-4158-af22-35d2448d0b13 --- pkg/inventory/filters.go | 10 ++++++---- pkg/inventory/registry_test.go | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 1e65b55685..abdafe9b2b 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -61,10 +61,12 @@ func (r *Inventory) isToolEnabled(ctx context.Context, tool *ServerTool, feature return false } if availability := tool.availability(); !availability.unrestricted() { - if info, ok := ghcontext.MCPMethod(ctx); ok && - (info.Method == MCPMethodToolsList || info.Method == MCPMethodToolsCall) && - !toolAvailable(info.ProtocolVersion, info.ClientCapabilities, availability) { - return false + if info, ok := ghcontext.MCPMethod(ctx); ok && (info.Method == MCPMethodToolsList || info.Method == MCPMethodToolsCall) { + known := (availability.minimumProtocolVersion == "" || info.ProtocolVersion != "") && + (availability.requiredElicitationMode == "" || info.ClientCapabilities != nil) + if known && !toolAvailable(info.ProtocolVersion, info.ClientCapabilities, availability) { + return false + } } } // 4. Check feature availability. diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 818692d189..9f359e8dce 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1959,6 +1959,11 @@ func TestMCPAvailabilityRunsBeforeFeatureChecks(t *testing.T) { }) require.Len(t, inv.AvailableTools(ctx), 2) require.Equal(t, 1, calls) + + calls = 0 + ctx = ghcontext.WithMCPMethodInfo(context.Background(), &ghcontext.MCPMethodInfo{Method: method}) + require.Len(t, inv.AvailableTools(ctx), 2) + require.Equal(t, 1, calls) }) } }