diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index cfb68be4e..72caa17cb 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -83,6 +83,7 @@ var ( LogFilePath: viper.GetString("log-file"), ContentWindowSize: viper.GetInt("content-window-size"), LockdownMode: viper.GetBool("lockdown-mode"), + Experimental: viper.GetBool("experimental"), RepoAccessCacheTTL: &ttl, } return ghmcp.RunStdioServer(stdioServerConfig) @@ -108,6 +109,7 @@ func init() { rootCmd.PersistentFlags().String("gh-host", "", "Specify the GitHub hostname (for GitHub Enterprise etc.)") rootCmd.PersistentFlags().Int("content-window-size", 5000, "Specify the content window size") rootCmd.PersistentFlags().Bool("lockdown-mode", false, "Enable lockdown mode") + rootCmd.PersistentFlags().Bool("experimental", false, "Enable experimental features") rootCmd.PersistentFlags().Duration("repo-access-cache-ttl", 5*time.Minute, "Override the repo access cache TTL (e.g. 1m, 0s to disable)") // Bind flag to viper @@ -122,6 +124,7 @@ func init() { _ = viper.BindPFlag("host", rootCmd.PersistentFlags().Lookup("gh-host")) _ = viper.BindPFlag("content-window-size", rootCmd.PersistentFlags().Lookup("content-window-size")) _ = viper.BindPFlag("lockdown-mode", rootCmd.PersistentFlags().Lookup("lockdown-mode")) + _ = viper.BindPFlag("experimental", rootCmd.PersistentFlags().Lookup("experimental")) _ = viper.BindPFlag("repo-access-cache-ttl", rootCmd.PersistentFlags().Lookup("repo-access-cache-ttl")) // Add subcommands diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 250f6b4cc..94d7c2794 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -64,6 +64,9 @@ type MCPServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool + // Experimental indicates if we should enable experimental features + Experimental bool + // Logger is used for logging within the server Logger *slog.Logger // RepoAccessTTL overrides the default TTL for repository access cache entries. @@ -198,6 +201,9 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { ghServer.AddReceivingMiddleware(addGitHubAPIErrorToContext) ghServer.AddReceivingMiddleware(addUserAgentsMiddleware(cfg, clients.rest, clients.gqlHTTP)) + // Create feature checker + featureChecker := createFeatureChecker(cfg.EnabledFeatures) + // Create dependencies for tool handlers deps := github.NewBaseDeps( clients.rest, @@ -205,8 +211,12 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { clients.raw, clients.repoAccess, cfg.Translator, - github.FeatureFlags{LockdownMode: cfg.LockdownMode}, + github.FeatureFlags{ + LockdownMode: cfg.LockdownMode, + Experimental: cfg.Experimental, + }, cfg.ContentWindowSize, + featureChecker, ) // Inject dependencies into context for all tool handlers @@ -222,7 +232,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { WithReadOnly(cfg.ReadOnly). WithToolsets(enabledToolsets). WithTools(github.CleanTools(cfg.EnabledTools)). - WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures)) + WithFeatureChecker(featureChecker) // Apply token scope filtering if scopes are known (for PAT filtering) if cfg.TokenScopes != nil { @@ -322,6 +332,9 @@ type StdioServerConfig struct { // LockdownMode indicates if we should enable lockdown mode LockdownMode bool + // Experimental indicates if we should enable experimental features + Experimental bool + // RepoAccessCacheTTL overrides the default TTL for repository access cache entries. RepoAccessCacheTTL *time.Duration } @@ -378,6 +391,7 @@ func RunStdioServer(cfg StdioServerConfig) error { Translator: t, ContentWindowSize: cfg.ContentWindowSize, LockdownMode: cfg.LockdownMode, + Experimental: cfg.Experimental, Logger: logger, RepoAccessTTL: cfg.RepoAccessCacheTTL, TokenScopes: tokenScopes, diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 04c0989d4..be4f914c7 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -23,6 +23,7 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { Translator: translations.NullTranslationHelper, ContentWindowSize: 5000, LockdownMode: false, + Experimental: false, } // Create the server diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index b41bf0b87..15d807a24 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -3,6 +3,8 @@ package github import ( "context" "errors" + "fmt" + "os" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" @@ -77,6 +79,9 @@ type ToolDependencies interface { // GetContentWindowSize returns the content window size for log truncation GetContentWindowSize() int + + // IsFeatureEnabled checks if a feature flag is enabled. + IsFeatureEnabled(ctx context.Context, flagName string) bool } // BaseDeps is the standard implementation of ToolDependencies for the local server. @@ -93,8 +98,14 @@ type BaseDeps struct { T translations.TranslationHelperFunc Flags FeatureFlags ContentWindowSize int + + // Feature flag checker for runtime checks + featureChecker inventory.FeatureFlagChecker } +// Compile-time assertion to verify that BaseDeps implements the ToolDependencies interface. +var _ ToolDependencies = (*BaseDeps)(nil) + // NewBaseDeps creates a BaseDeps with the provided clients and configuration. func NewBaseDeps( client *gogithub.Client, @@ -104,6 +115,7 @@ func NewBaseDeps( t translations.TranslationHelperFunc, flags FeatureFlags, contentWindowSize int, + featureChecker inventory.FeatureFlagChecker, ) *BaseDeps { return &BaseDeps{ Client: client, @@ -113,6 +125,7 @@ func NewBaseDeps( T: t, Flags: flags, ContentWindowSize: contentWindowSize, + featureChecker: featureChecker, } } @@ -143,6 +156,24 @@ func (d BaseDeps) GetFlags() FeatureFlags { return d.Flags } // GetContentWindowSize implements ToolDependencies. func (d BaseDeps) GetContentWindowSize() int { return d.ContentWindowSize } +// 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 +} + // NewTool creates a ServerTool that retrieves ToolDependencies from context at call time. // This avoids creating closures at registration time, which is important for performance // in servers that create a new server instance per request (like the remote server). diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go new file mode 100644 index 000000000..d13160d4c --- /dev/null +++ b/pkg/github/dependencies_test.go @@ -0,0 +1,108 @@ +package github_test + +import ( + "context" + "errors" + "testing" + + "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/stretchr/testify/assert" +) + +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) { + return flagName == "test_flag", nil + } + + // Create deps with the checker using NewBaseDeps + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Test enabled flag + result := deps.IsFeatureEnabled(context.Background(), "test_flag") + assert.True(t, result, "Expected test_flag to be enabled") + + // Test disabled flag + result = deps.IsFeatureEnabled(context.Background(), "other_flag") + assert.False(t, result, "Expected other_flag to be disabled") +} + +func TestIsFeatureEnabled_WithoutChecker(t *testing.T) { + t.Parallel() + + // Create deps without feature checker (nil) + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + nil, // featureChecker (nil) + ) + + // Should return false when checker is nil + result := deps.IsFeatureEnabled(context.Background(), "any_flag") + assert.False(t, result, "Expected false when checker is nil") +} + +func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) { + t.Parallel() + + // Create a feature checker + checker := func(_ context.Context, _ string) (bool, error) { + return true, nil + } + + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Should return false for empty flag name + result := deps.IsFeatureEnabled(context.Background(), "") + assert.False(t, result, "Expected false for empty flag name") +} + +func TestIsFeatureEnabled_CheckerError(t *testing.T) { + t.Parallel() + + // Create a feature checker that returns an error + checker := func(_ context.Context, _ string) (bool, error) { + return false, errors.New("checker error") + } + + deps := github.NewBaseDeps( + nil, // client + nil, // gqlClient + nil, // rawClient + nil, // repoAccessCache + translations.NullTranslationHelper, + github.FeatureFlags{}, + 0, // contentWindowSize + checker, // featureChecker + ) + + // Should return false and log error (not crash) + result := deps.IsFeatureEnabled(context.Background(), "error_flag") + assert.False(t, result, "Expected false when checker returns error") +} diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go index 8d12b78c2..3fc701eb4 100644 --- a/pkg/github/dynamic_tools_test.go +++ b/pkg/github/dynamic_tools_test.go @@ -133,7 +133,7 @@ func TestDynamicTools_EnableToolset(t *testing.T) { deps := DynamicToolDependencies{ Server: server, Inventory: reg, - ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0), + ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil), T: translations.NullTranslationHelper, } diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 047042e44..e11847194 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -3,4 +3,5 @@ package github // FeatureFlags defines runtime feature toggles that adjust tool behavior. type FeatureFlags struct { LockdownMode bool + Experimental bool } diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go new file mode 100644 index 000000000..61caeb2e8 --- /dev/null +++ b/pkg/github/feature_flags_test.go @@ -0,0 +1,199 @@ +package github + +import ( + "context" + "encoding/json" + "testing" + + "github.com/github/github-mcp-server/pkg/translations" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/utils" +) + +// RemoteMCPExperimental is a long-lived feature flag for experimental remote MCP features. +// This flag enables experimental behaviors in tools that are being tested for remote server deployment. +const RemoteMCPEnthusiasticGreeting = "remote_mcp_enthusiastic_greeting" + +// FeatureChecker is an interface for checking if a feature flag is enabled. +type FeatureChecker interface { + // IsFeatureEnabled checks if a feature flag is enabled. + IsFeatureEnabled(ctx context.Context, flagName string) bool +} + +// HelloWorld returns a simple greeting tool that demonstrates feature flag conditional behavior. +// This tool is for testing and demonstration purposes only. +func HelloWorldTool(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataContext, // Use existing "context" toolset + mcp.Tool{ + Name: "hello_world", + Description: t("TOOL_HELLO_WORLD_DESCRIPTION", "A simple greeting tool that demonstrates feature flag conditional behavior"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_HELLO_WORLD_TITLE", "Hello World"), + ReadOnlyHint: true, + }, + }, + []scopes.Scope{}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { + + // Check feature flag to determine greeting style + greeting := "Hello, world!" + if deps.IsFeatureEnabled(ctx, RemoteMCPEnthusiasticGreeting) { + greeting += " Welcome to the future of MCP! 🎉" + } + if deps.GetFlags().Experimental { + greeting += " Experimental features are enabled! 🚀" + } + + // Build response + response := map[string]any{ + "greeting": greeting, + } + + jsonBytes, err := json.Marshal(response) + if err != nil { + return utils.NewToolResultError("failed to marshal response"), nil, nil + } + + return utils.NewToolResultText(string(jsonBytes)), nil, nil + }, + ) +} + +func TestHelloWorld_ConditionalBehavior_Featureflag(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + featureFlagEnabled bool + inputName string + expectedGreeting string + }{ + { + name: "Feature flag disabled - default greeting", + featureFlagEnabled: false, + expectedGreeting: "Hello, world!", + }, + { + name: "Feature flag enabled - enthusiastic greeting", + featureFlagEnabled: true, + expectedGreeting: "Hello, world! Welcome to the future of MCP! 🎉", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Create feature checker based on test case + checker := func(_ context.Context, flagName string) (bool, error) { + if flagName == RemoteMCPEnthusiasticGreeting { + return tt.featureFlagEnabled, nil + } + return false, nil + } + + // Create deps with the checker + deps := NewBaseDeps( + nil, nil, nil, nil, + translations.NullTranslationHelper, + FeatureFlags{}, + 0, + checker, + ) + + // Get the tool and its handler + tool := HelloWorldTool(translations.NullTranslationHelper) + handler := tool.Handler(deps) + + // Call the handler with deps in context + ctx := ContextWithDeps(context.Background(), deps) + result, err := handler(ctx, &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Arguments: json.RawMessage(`{}`), + }, + }) + require.NoError(t, err) + require.NotNil(t, result) + require.Len(t, result.Content, 1) + + // Parse the response - should be TextContent + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected content to be TextContent") + + var response map[string]any + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err) + + // Verify the greeting matches expected based on feature flag + assert.Equal(t, tt.expectedGreeting, response["greeting"]) + }) + } +} + +func TestHelloWorld_ConditionalBehavior_Config(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + experimental bool + expectedGreeting string + }{ + { + name: "Experimental disabled - default greeting", + experimental: false, + expectedGreeting: "Hello, world!", + }, + { + name: "Experimental enabled - experimental greeting", + experimental: true, + expectedGreeting: "Hello, world! Experimental features are enabled! 🚀", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Create deps with the checker + deps := NewBaseDeps( + nil, nil, nil, nil, + translations.NullTranslationHelper, + FeatureFlags{Experimental: tt.experimental}, + 0, + nil, + ) + + // Get the tool and its handler + tool := HelloWorldTool(translations.NullTranslationHelper) + handler := tool.Handler(deps) + + // Call the handler with deps in context + ctx := ContextWithDeps(context.Background(), deps) + result, err := handler(ctx, &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Arguments: json.RawMessage(`{}`), + }, + }) + require.NoError(t, err) + require.NotNil(t, result) + require.Len(t, result.Content, 1) + + // Parse the response - should be TextContent + textContent, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok, "expected content to be TextContent") + + var response map[string]any + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err) + + // Verify the greeting matches expected based on feature flag + assert.Equal(t, tt.expectedGreeting, response["greeting"]) + }) + } +} diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index a59cd9a93..1d07a0de5 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -51,10 +51,11 @@ func (s stubDeps) GetRawClient(ctx context.Context) (*raw.Client, error) { return nil, nil } -func (s stubDeps) GetRepoAccessCache() *lockdown.RepoAccessCache { return s.repoAccessCache } -func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t } -func (s stubDeps) GetFlags() FeatureFlags { return s.flags } -func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } +func (s stubDeps) GetRepoAccessCache() *lockdown.RepoAccessCache { return s.repoAccessCache } +func (s stubDeps) GetT() translations.TranslationHelperFunc { return s.t } +func (s stubDeps) GetFlags() FeatureFlags { return s.flags } +func (s stubDeps) GetContentWindowSize() int { return s.contentWindowSize } +func (s stubDeps) IsFeatureEnabled(_ context.Context, _ string) bool { return false } // Helper functions to create stub client functions for error testing func stubClientFnFromHTTP(httpClient *http.Client) func(context.Context) (*github.Client, error) { @@ -83,6 +84,7 @@ func stubRepoAccessCache(client *githubv4.Client, ttl time.Duration) *lockdown.R func stubFeatureFlags(enabledFlags map[string]bool) FeatureFlags { return FeatureFlags{ LockdownMode: enabledFlags["lockdown-mode"], + Experimental: enabledFlags["experimental"], } }