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

Author: SamMorrowDrums

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"}).

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
 	}
 }
 

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

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",
 		},

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)
 		}
 	})
 }

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)

pkg/github/pullrequests.go

@@ -1908,21 +1908,21 @@ 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 {
 		if cfg.hostType == utils.HostTypeGHES {
 			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))
 				},
 			)
 		}

pkg/github/pullrequests_granular.go

@@ -751,19 +751,19 @@ 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)
 			},
 		)
 	case cfg.hostType == utils.HostTypeGHES:
 		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)
 			},
 		)